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

Add configurable operation listener for translog #92926

Merged
merged 9 commits into from
Jan 24, 2023

Conversation

Tim-Brooks
Copy link
Contributor

This commit adds a listener that will be called when new operations are written to the translog writer.

This commit adds a listener that will be called when new operations are
written to the translog writer.
@Tim-Brooks Tim-Brooks added >non-issue :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. v8.7.0 labels Jan 16, 2023
@elasticsearchmachine elasticsearchmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Jan 16, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

@FunctionalInterface
public interface OperationListener {

void operationAdded(BytesReference data, long seqNo, Translog.Location location);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we expecting to add more operations to the listener?
Otherwise would it be better to use action listener with a record to hold all of the above arguments?

Copy link
Contributor Author

@Tim-Brooks Tim-Brooks Jan 19, 2023

Choose a reason for hiding this comment

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

I mean I'm not sure we should use an ActionListener as we do not have a failure condition here. I understand that we could use a Consumer<CustomRecord> but in that scenario we are still adding an abstraction (record vs. functional interface). So I guess I don't see the advantage to it.

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

Looks ok to me, but could we add some tests that show the listener is called in all the places we expect it to be called?

@Tim-Brooks
Copy link
Contributor Author

Looks ok to me, but could we add some tests that show the listener is called in all the places we expect it to be called?

yes definitely. Sorry this is for sure the main thing outstanding on this PR that I have been meaning todo. I'll add them later today.

@arteam arteam self-requested a review January 23, 2023 12:54
Copy link
Contributor

@arteam arteam left a comment

Choose a reason for hiding this comment

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

LGTM! I've left a few small comments

OperationListener listener = (data, seqNo, location) -> {
seqNos.add(seqNo);
locations.add(location);
try (BytesStreamOutput output = new BytesStreamOutput()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need to copy the datum or we can just call datas.add(data)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The data in the listener is a ReleasableBytesReference from the Translog that will be released once the method returns. So yes. Obviously in tests it does not matter much, but it does matter for real usages.

I added documentation around the listener for this point.

@Tim-Brooks Tim-Brooks merged commit 45ed328 into elastic:main Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. >non-issue Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants