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

DynamoDB: Fix parallelism #876 #877

Merged
merged 1 commit into from
Apr 11, 2018
Merged

DynamoDB: Fix parallelism #876 #877

merged 1 commit into from
Apr 11, 2018

Conversation

gregbeech
Copy link
Contributor

@gregbeech gregbeech commented Apr 6, 2018

This PR fixes #876 by changing the parallelism in DynamoDB so that it is applied to the HTTP connection pool.

I upped the default parallelism to 32 as this is the nearest power of 2 below 50, which is the default that AWS used in their own SDK before deprecating the methods. I've run up to 128 connections in production (by setting the akka-http config properties) without any problems so 32 seems perfectly safe.

This PR fixes the parallelism in DynamoDB so that it is applied to the HTTP connection pool.

I upped the default parallelism to 32 as this is the nearest power of 2 below 50, which is the default that [AWS used in their own SDK before deprecating the methods](https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/services/dynamodbv2/AmazonDynamoDBAsyncClient.html). I've run up to 128 connections in production (by setting the akka-http config propertis) without any problems so 32 seems perfectly safe.
Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me.
Where does the need for "power of two" come from?

@gregbeech
Copy link
Contributor Author

The power of two is from the akka-http requirement for max open requests:

    # Must be a power of 2 and > 0!
    max-open-requests = 32

It's an intriguing requirement but I haven't dug into the code enough to see why it's implemented that way.

@gregbeech
Copy link
Contributor Author

One thing that occurs to me is that with this change parallelism is still used for mapAsync and it's likely way higher than needed.

For example, we use 128 connections from our production servers but don't need anything like that parallelism for mapAsync as it's so much faster (we still use the default of 10 for that). Having another setting for that value seems messy though, as that's really an implementation detail that the user of the library shouldn't have to know/care about.

Any ideas? Perhaps leaving them the same is fine as it'll never get that high, or we could compute a value for it that seems more reasonable such as parallelism/8.

@ennru
Copy link
Member

ennru commented Apr 11, 2018

I think its OK to use the same high value for the mapAsync instead of complicating the settings or the explanations thereof.

Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ennru ennru merged commit 6610a7d into akka:master Apr 11, 2018
@ennru
Copy link
Member

ennru commented Apr 11, 2018

Thank you for this improvement. Good to see you funnelling quite some data through it!

@ennru ennru added this to the 0.19 milestone Apr 11, 2018
@gregbeech gregbeech deleted the fix-dynamodb-parallelism-876 branch April 11, 2018 09:18
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.

AWS DynamoDB doesn't obey parallelism setting
2 participants