-
Notifications
You must be signed in to change notification settings - Fork 10
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
Issue 3817 Update default values and descriptions for concurrency properties #3820
base: develop
Are you sure you want to change the base?
Issue 3817 Update default values and descriptions for concurrency properties #3820
Conversation
@@ -347,11 +347,11 @@ public interface DefaultProperty { | |||
.propertyGroup(InstancePropertyGroup.DEFAULT).build(); | |||
UserDefinedInstanceProperty DEFAULT_LAMBDA_CONCURRENCY_MAXIMUM = Index.propertyBuilder("sleeper.default.lambda.concurrency.max") | |||
.description("Default value for the maximum concurrency for each lambda within the Sleeper instance. " + | |||
"By default the maximum concurrency is unlimited. Each lambda also has its own property that " + | |||
"By default the maximum concurrency is set 10 concurrent tables. Each lambda also has its own property that " + |
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.
I think this would read better as "is set to 10 which is enough for 10 online tables. If there are more online tables this number may need to be increased".
"See reserved concurrency overview at: https://docs.aws.amazon.com/lambda/latest/dg/configuration-concurrency.html") | ||
.defaultValue("10") |
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.
This isn't being applied because the default property is still set. That needs to be removed.
@@ -358,7 +358,9 @@ public interface CommonProperty { | |||
UserDefinedInstanceProperty STATESTORE_COMMITTER_LAMBDA_CONCURRENCY_RESERVED = Index.propertyBuilder("sleeper.statestore.committer.concurrency.reserved") | |||
.defaultProperty(DEFAULT_LAMBDA_CONCURRENCY_RESERVED) | |||
.description("The reserved concurrency for the state store committer lambda.\n" + | |||
"Presently this value defaults to 10 to align with expectations around table efficiency. \n" + |
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.
We could explain this is to ensure state store operations can be applied to at least 10 tables even when concurrency is used up in the account.
Make sure you have checked all steps below.
Issue
PR"
Tests
Documentation
separate issue for that below.