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

limit the default max batch size #95

Merged
merged 1 commit into from
Jun 9, 2015
Merged

Conversation

fwbrasil
Copy link
Contributor

@fwbrasil fwbrasil commented Jun 5, 2015

@williamboxhall @stevenheidel

I spent many days trying to identify the cause for a production issue and in the end the main problem was the size of the batch requests to memcached. The system was making requests for batches with thousands of keys.

We need to avoid letting users run into similar problems. This pull request sets the default maxBatchSize to 100. Another alternative would be make the maxBatchSize required when creating a ClumpSource.

@stevenheidel
Copy link
Contributor

I like this method, making the max batch size required would be too onerous on most use cases.

@williamboxhall
Copy link
Contributor

can you think of a valid case where someone would want to use more than 100 / or use an unlimited batch size?

@fwbrasil
Copy link
Contributor Author

fwbrasil commented Jun 8, 2015

I imagine that some cases would need bigger batches, but I think they would be the minority. IMHO having to define a custom batch size using source.maxBatchSize is reasonable for these cases.

@williamboxhall
Copy link
Contributor

👍 it's a good default behaviour, one less thing for developers to need to think about out of the box

williamboxhall added a commit that referenced this pull request Jun 9, 2015
limit the default max batch size
@williamboxhall williamboxhall merged commit 0f7b50a into master Jun 9, 2015
@williamboxhall williamboxhall deleted the limit-batch-size branch June 9, 2015 08:00
@williamboxhall
Copy link
Contributor

merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants