[Kinesis] add Scala kinesis-mock build behind feature flag #12559
Conversation
S3 Image Test Results (AMD64 / ARM64) 2 files 2 suites 8m 41s ⏱️ Results for commit a089a68. ♻️ This comment has been updated with latest results. |
65c9b9a to
a400993
Compare
a400993 to
4eed69f
Compare
| "-XX:+UseG1GC", | ||
| "-XX:MaxGCPauseMillis=500", | ||
| "-XX:+UseGCOverheadLimit", | ||
| "-XX:+ExplicitGCInvokesConcurrent", | ||
| "-XX:+HeapDumpOnOutOfMemoryError", | ||
| "-XX:+ExitOnOutOfMemoryError", |
There was a problem hiding this comment.
Do we really need to set all these flags? Do they improve performance in any way? Especially the heap dump seems like something our customers do not want, and G1GC is generally the default GC in all recent java versions.
There was a problem hiding this comment.
The MaxGCPauseMillis was for performance reasons and the ExitOnOutOfMemoryError was because I didn't want the server to stall when encountering an OOM.
Will remove:
UseG1GCsince I was unaware it was the default (as per your comment)UseGCOverheadLimitsince it does nothing withG1GCHeapDumpOnOutOfMemoryErrorsince it'll spam the logs quite a bit
| @cached_property | ||
| def engine(self) -> KinesisMockEngine: | ||
| return KinesisMockEngine(config.KINESIS_MOCK_PROVIDER_ENGINE) | ||
|
|
There was a problem hiding this comment.
To determine which Kinesis mock backend to run:
localstack/localstack-core/localstack/services/kinesis/kinesis_mock_server.py
Lines 208 to 225 in 4eed69f
There was a problem hiding this comment.
In this case - I am not that happy with the control here. The Servers should control which package is installed, not the package which server is used, in my opinion.
Could simplify this a bit, by having two packages, and installing the right one depending on the configuration, and then use the right server?
There was a problem hiding this comment.
Yeah sure happy to have 2 packages 👍
There was a problem hiding this comment.
@dfangl How should we go about configuring the plugin for the LPM?
Should we have a seperate kinesis-mock and kinesis-mock-scala plugin or should this implementation be unified to cater for both node + scala builds i.e
@package(name="kinesis-mock")
def kinesismock_package() -> Package:
from localstack.services.kinesis.packages import kinesismock_package
return kinesismock_package
@package(name="kinesis-mock-scala")
def kinesismock_scala_package() -> Package:
from localstack.services.kinesis.packages import kinesismock_scala_package
return kinesismock_scala_packageOR
@package(name="kinesis-mock")
def kinesismock_package() -> Package:
from localstack.services.kinesis.packages import (
KinesisMockEngine,
kinesismock_package,
kinesismock_scala_package,
)
if KinesisMockEngine(config.KINESIS_MOCK_PROVIDER_ENGINE) == KinesisMockEngine.SCALA:
return kinesismock_scala_package
return kinesismock_packageThere was a problem hiding this comment.
@dfangl FYI I spoke to Alex re: the above and we went with the second approach ☝️ Nvm: going to comment on your latest review
4eed69f to
cc634d2
Compare
There was a problem hiding this comment.
LGTM! Way cleaner than before in my opinion 👍
| def _missing_(cls, value: str | Any) -> str: | ||
| # default to 'node' if invalid enum | ||
| if not isinstance(value, str): | ||
| return cls(cls.NODE) | ||
| return cls.__members__.get(value.upper(), cls.NODE) |
There was a problem hiding this comment.
Nit: Not entirely happy with this fallback logic here - it is effectively unused, as we only ever check if it is scala anyway, and we would silently accept obviously wrong values. Also, if we ever switch the default, we have to do it in multiple places. Nothing to hold back the merge, but I think we could potentially make this easier.
There was a problem hiding this comment.
Yeah I suppose this is unideal but I wanted to make sure (since we're feature-flagging this strategy) that nothing gets broken if incorrect values are used. Definitely erring on the defensive side.
What have we done in past for situations like this?
There was a problem hiding this comment.
Usually we would fallback, but at least with a warning in the log.
Motivation
We've observed the
NodeJSbuild of kinesis mock to encounter performance issues at higher volumes of requests -- namely with larger payload sizes.This PR adds the original Scala build as an alternative engine for our Kinesis server behind the
KINESIS_MOCK_PROVIDER_ENGINEflag.Notes
Changes
Functionality
KinesisMockServerto include aKinesisMockScalaServerandKinesisMockNodeServerKinesisMockPackagecan now either use theKinesisMockScalaPackageInstaller(downloads from GitHub) orKinesisMockNodePackageInstaller(downloads from NPM)Config
KINESIS_MOCK_PROVIDER_ENGINEflag for switching to Scala build of Kinesis Mock. Valid values arenodeorscala-- where empty or invalid values will always default tonode.KINESIS_MOCK_MAXIMUM_HEAP_SIZEsets the maximum Java heap size corresponding to the '-Xmx' flagKINESIS_MOCK_INITIAL_HEAP_SIZEsets the initial Java heap size corresponding to the '-Xms' flagTesting
TestKinesisMockScala(TestKinesis)subclass that runs all tests intests/aws/services/kinesis/test_kinesis.py::TestKinesisbut with the configuration valuemonkeypatchedto use the Scala build of Kinesis mock instead of NodeJS.