-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Minor cleanups FrozenIndexInput #93309
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
Minor cleanups FrozenIndexInput #93309
Conversation
Some random finds while working with this code. We shouldn't use a Consumer<Long> instead of a LongConsumer as we never pass `null` to the consumer. Also, way simplified the locking around the Lucene `Bytebuffer b` to simplify the code and technically make it a little faster/less-contenting as well. Plus, made use of modern Java's buffer slicing to simplify the slicing of the Lucene buffer.
|
Pinging @elastic/es-distributed (Team:Distributed) |
| } finally { | ||
| preventAsyncBufferChanges.run(); | ||
| if (bufferWriteLocked == false) { | ||
| luceneByteBufPermits.acquire(Integer.MAX_VALUE); |
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.
Did you intend to release here?
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.
No this is just a paranoid fail-safe like we had in the previous version of this, we never release. We always just acquire all of them to close the buffer for writes for good to have the same behaviour we previously had via the boolean but simpler.
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.
I see, thanks. I wonder if we could have some sort of ByteBuffer wrapper that we could "invalidate" such at it then prevent any reading from it.
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.
(not blocking the PR to be merged)
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.
Let's see later I'd say. This is definitely a pattern we don't just have here but also in at least one other spot. I'm still hoping maybe we can find a way to not have to do this (passing the buffer around to other threads in general) since somehow this is never actually safe :) I'll think on it!
tlrx
left a comment
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 Tanguy! |
Some random finds while working with this code. We shouldn't use a Consumer<Long> instead of a LongConsumer as we never pass `null` to the consumer. Also, way simplified the locking around the Lucene `Bytebuffer b` to simplify the code and technically make it a little faster/less-contenting as well. Plus, made use of modern Java's buffer slicing to simplify the slicing of the Lucene buffer.
Some random finds while working with this code. We shouldn't use a Consumer instead of a LongConsumer as we never pass
nullto the consumer.Also, way simplified the locking around the Lucene
Bytebuffer bto simplify the code and technically make it a little faster/less-contenting as well.Plus, made use of modern Java's buffer slicing to simplify the slicing of the Lucene buffer.