Skip to content

LUCENE-9406: Add IndexWriterEventListener to track events in IndexWriter #2342

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

Merged

Conversation

zacharymorn
Copy link
Contributor

Description / Solution

Add IndexWriterEventListener interface to track events in IndexWriter

Tests

  • Passed existing tests
  • Added an additional test

Note: precommit is currently failing due to nocommit comment

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
  • I have developed this patch against the master branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Ref Guide (for Solr changes only).

MergePolicy.OneMerge nextMerge = null;

if (pendingMerges.size() > 0) {
// nocommit getting OneMerge instance here via mergeSource.getNextMerge() will
Copy link
Member

Choose a reason for hiding this comment

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

How about passing the pendingMerges to the event listener instead? (Sorry if I asked for OneMerge on the issue! MergeSpecification is better since it can hold multiple merges, allows event listener to log things like how many merges were requested during commit, etc.).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem! From the context I assume you are actually meaning to pass pointInTimeMerges into event listener, since pendingMerges is of type Deque<MergePolicy.OneMerge>? I've pushed a commit to use pointInTimeMerges for now.

}

// Test basic semantics of merge on commit and events recording invocation
public void testMergeOnCommitWithEventListener() throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

You might also edit LuceneTestCase.newIndexWriterConfig to randomly swap in a MockIndexWriterEventListener just to exercise this listener in any tests using that API, which is quite a few. It could uncover times when we accidentally break something when this listener is invoked ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Added.

…r randomly into LuceneTestCase.newIndexConfig
@dweiss
Copy link
Contributor

dweiss commented Feb 12, 2021

I think it's good overall but I'm wondering whether it makes sense to make that field volatile... do we want to allow changing listeners over index writer lifecycle? I think it should be a regular field and IW should just read it once (and set forever).

@zacharymorn
Copy link
Contributor Author

I think it's good overall but I'm wondering whether it makes sense to make that field volatile... do we want to allow changing listeners over index writer lifecycle? I think it should be a regular field and IW should just read it once (and set forever).

Thanks for the feedback Dawid! I thought about this a bit as well when I noticed most of the other fields are volatile, but I can't decide for sure which direction to go as I don't have enough context information about the different use cases IW may need to support in the wild, so I ended up following the existing pattern here. However, I do feel that it may not hurt for application to have the freedom to switch event listener in the middle if situation or need dictates (i.e. a service application containing Lucene may need to switch event listener if it exposes an API for its client to choose how the IW event stream to be sent and stored) ?

@dweiss
Copy link
Contributor

dweiss commented Feb 13, 2021

If such a situation arises folks will have the ability to write a custom proxy listener with the ability to switch, add and/or remove delegates. This keeps the freedom to do anything you said and at the same time simplifies implementation (and concurrency-issues) on IW side?

@zacharymorn
Copy link
Contributor Author

If such a situation arises folks will have the ability to write a custom proxy listener with the ability to switch, add and/or remove delegates. This keeps the freedom to do anything you said and at the same time simplifies implementation (and concurrency-issues) on IW side?

Ah that's very true! I've pushed a commit to remove volatile then. Please let me know if it looks good.

Copy link
Contributor

@dweiss dweiss left a comment

Choose a reason for hiding this comment

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

Yep, looks good to me!

@zacharymorn
Copy link
Contributor Author

zacharymorn commented Feb 26, 2021

Circling back to this PR as I just realized I have forgotten about it.. :D

Thanks @dweiss for the review!

@mikemccand do you think this PR is ready now? Anything else you would like me to improve on?

@dweiss dweiss merged commit 6ba9fe5 into apache:master Mar 2, 2021
@dweiss
Copy link
Contributor

dweiss commented Mar 2, 2021

Looks good to me. I've merged this in, sorry for the delay.

@zacharymorn
Copy link
Contributor Author

No worry! Thanks for the review and merge!

@mikemccand
Copy link
Member

Woops, sorry for the belated response, and thank you @zacharymorn for creating this and @dweiss for merging -- it looks great! We can now add other events to track incrementally over time ...

@zacharymorn
Copy link
Contributor Author

No problem, and thanks for the review feedback as well Michael!

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