-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
[Kinesis] add Scala kinesis-mock build behind feature flag #12559
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
UseG1GC
since I was unaware it was the default (as per your comment)UseGCOverheadLimit
since it does nothing withG1GC
HeapDumpOnOutOfMemoryError
since 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To determine which Kinesis mock backend to run:
localstack/localstack-core/localstack/services/kinesis/kinesis_mock_server.py
Lines 208 to 225 in 4eed69f
if kinesismock_package.engine == KinesisMockEngine.SCALA: | |
return KinesisMockScalaServer( | |
port=port, | |
exe_path=kinesis_mock_path, | |
log_level=log_level, | |
latency=latency, | |
data_dir=persist_path, | |
account_id=account_id, | |
) | |
return KinesisMockNodeServer( | |
port=port, | |
exe_path=kinesis_mock_path, | |
log_level=log_level, | |
latency=latency, | |
data_dir=persist_path, | |
account_id=account_id, | |
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sure happy to have 2 packages 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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_package
OR
@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_package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually we would fallback, but at least with a warning in the log.
Motivation
We've observed the
NodeJS
build 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_ENGINE
flag.Notes
Changes
Functionality
KinesisMockServer
to include aKinesisMockScalaServer
andKinesisMockNodeServer
KinesisMockPackage
can now either use theKinesisMockScalaPackageInstaller
(downloads from GitHub) orKinesisMockNodePackageInstaller
(downloads from NPM)Config
KINESIS_MOCK_PROVIDER_ENGINE
flag for switching to Scala build of Kinesis Mock. Valid values arenode
orscala
-- where empty or invalid values will always default tonode
.KINESIS_MOCK_MAXIMUM_HEAP_SIZE
sets the maximum Java heap size corresponding to the '-Xmx' flagKINESIS_MOCK_INITIAL_HEAP_SIZE
sets the initial Java heap size corresponding to the '-Xms' flagTesting
TestKinesisMockScala(TestKinesis)
subclass that runs all tests intests/aws/services/kinesis/test_kinesis.py::TestKinesis
but with the configuration valuemonkeypatched
to use the Scala build of Kinesis mock instead of NodeJS.