Skip to content
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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

rtjd6554
Copy link
Collaborator

@rtjd6554 rtjd6554 commented Nov 28, 2024

Make sure you have checked all steps below.

Issue

Tests

  • My PR adds the following tests OR does not need testing for this extremely good reason:
    • Build of application

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it, or I have linked to a
    separate issue for that below.
  • If I have added or removed any dependencies from the project, I have updated the NOTICES file.

@rtjd6554 rtjd6554 self-assigned this Nov 28, 2024
@rtjd6554 rtjd6554 marked this pull request as ready for review November 28, 2024 10:16
@rtjd6554 rtjd6554 assigned patchwork01 and unassigned rtjd6554 Nov 28, 2024
@@ -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 " +
Copy link
Member

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")
Copy link
Collaborator

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" +
Copy link
Collaborator

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.

@patchwork01 patchwork01 assigned rtjd6554 and unassigned patchwork01 Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set default max concurrency, reserved concurrency for state store committer
3 participants