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

change batch delay to 50ms and page size to 64mb #8797

Merged
merged 1 commit into from Dec 5, 2017

Conversation

Projects
None yet
5 participants
@colinsurprenant
Copy link
Contributor

commented Dec 4, 2017

Fixes #8707
Relates to #8702, #8674

Change the default values for pipeline.batch.delay from 5ms to 50ms and queue.page_capacity from 250mb to 64mb.

Also fixes documentation & comments inconsistencies.

@jordansissel

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2017

@colinsurprenant can you include some reasons/citation(s) in your commit message to explain this change? (for our future selves reading git log or similar)

@colinsurprenant

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2017

@jordansissel yes I can 👍

@colinsurprenant colinsurprenant force-pushed the colinsurprenant:fix/queue_defaults branch from 4d4598e to a44d3af Dec 4, 2017

@colinsurprenant

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2017

@jordansissel let me know if new commit message is better.

@jakelandis

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2017

@colinsurprenant - did we ever get to the bottom of why larger pages show less performance the further into the page ?

@yaauie

yaauie approved these changes Dec 4, 2017

Copy link
Member

left a comment

LGTM

@colinsurprenant

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2017

@jakelandis No. There is some investigation going on in #8688 but nothing specifically about the page size itself is clearly identified. I think a specific issue about this could be useful to track.

@jakelandis

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2017

@colinsurprenant - ok, was just curious. Would you mind logging that issue for the followup... perhaps framed as define the optimal page size ?

I don't want to block progress on an known performance issue and 64Mb feels reasonable.

LGTM2

change batch delay to 50ms and page size to 64mb
a 50ms delay helps creating more full batches without practical added latency see #8707
a 64mb page helps PQ perfmance related to a large page size see #8702 #8707

@colinsurprenant colinsurprenant force-pushed the colinsurprenant:fix/queue_defaults branch from a44d3af to 40827a5 Dec 5, 2017

@colinsurprenant

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2017

created #8801 to followup on the large page performance issue.

@colinsurprenant colinsurprenant merged commit 40827a5 into elastic:master Dec 5, 2017

1 of 2 checks passed

logstash-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details
@colinsurprenant

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2017

rebased, merged into master and 6.x (will be included in 6.2).
thanks @yaauie @jakelandis @jordansissel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.