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
Do not alloc full buffer for small change requests #35158
Conversation
Today we always allocate a full buffer (1024 elements) even though the requesting size is much smaller. With this change, we will use the requesting size as the buffer size if it's smaller than the default batch size; otherwise uses the default batch.
Pinging @elastic/es-distributed |
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.
LGTM.
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.
LGTM
@@ -95,14 +95,15 @@ | |||
} | |||
}; | |||
this.mapperService = mapperService; | |||
this.searchBatchSize = searchBatchSize; | |||
final long requestingSize = (toSeqNo - fromSeqNo) == Long.MAX_VALUE ? Long.MAX_VALUE : (toSeqNo - fromSeqNo + 1L); |
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.
(toSeqNo - fromSeqNo) == Long.MAX_VALUE
this would practically always evaluate to false
?
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.
Yes for CCR but there is another usage (i.e, peer recovery) where to_seq_no=MAX_VALUE and from_seq_no can be zero:
https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java#L496.
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.
Got it, thanks for explaining!
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.
LGTM
Thanks everyone :) |
Today we always allocate a full buffer (1024 elements) in a LuceneChangesSnapshot even though the requesting size is smaller. With this change, we will use the requesting size as the buffer size if it's smaller than the default batch size; otherwise uses the default batch size.
Today we always allocate a full buffer (1024 elements) in a LuceneChangesSnapshot even though the requesting size is smaller. With this change, we will use the requesting size as the buffer size if it's smaller than the default batch size; otherwise uses the default batch size.
Today we always allocate a full buffer (1024 elements) in a LuceneChangesSnapshot even though the requesting size is much smaller. With this change, we will use the requesting size as the buffer size if it's smaller than the default batch size; otherwise uses the default batch size.