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

fix: bulk ingester might skip listener requests #867

Merged
merged 4 commits into from
Sep 12, 2024

Conversation

fabriziofortino
Copy link
Contributor

When BulkIngester uses the internal scheduler it might fail notifying the listener because the scheduler gets terminated before the submit is called. This happens because the close condition is signalled before submit is invoked. This PR fixes a regression introduced in #830

Copy link

cla-checker-service bot commented Aug 20, 2024

💚 CLA has been signed

@fabriziofortino fabriziofortino changed the title fix: bulk ingester might skip lister requests fix: bulk ingester might skip listener requests Aug 20, 2024
fabriziofortino added a commit to fabriziofortino/jackrabbit-oak that referenced this pull request Aug 20, 2024
@l-trotta l-trotta self-assigned this Aug 21, 2024
fabriziofortino added a commit to apache/jackrabbit-oak that referenced this pull request Aug 22, 2024
* OAK-11042: bump elastic 8.15.0 / lucene 9.11.1

* OAK-11029: add workaround for elastic/elasticsearch-java#867

* OAK-11029: better doc for workaround

* OAK-11029: improve bulkIngester#close

* OAK-11029: missing static modifier for ElasticBulkProcessorHandler#FAILED_DOC_COUNT_FOR_STATUS_NODE
@l-trotta
Copy link
Contributor

Hello @fabriziofortino, thank you for the contribution and for highlighting this issue. I have tested your solution and while it helps mitigate the problem, when dealing with many requests there's still a possibility that the latest listener tasks are ignored because of the shutdown. I'll do more testing to figure out a way to handle this reliably, I'll let you know so that we can fix this PR and merge it.

@l-trotta
Copy link
Contributor

@fabriziofortino do you mind if I push on this branch?

@fabriziofortino
Copy link
Contributor Author

@l-trotta no problem, go ahead!

@fabriziofortino
Copy link
Contributor Author

@l-trotta I have also noticed this: in the close() method when the scheduler is internal shutdownNow() gets called:

       if (scheduler != null && !isExternalScheduler) {
            scheduler.shutdownNow();
        }

This will try to cancel the already submitted tasks. Shouldn't we call shutdown() instead?

@l-trotta
Copy link
Contributor

@fabriziofortino you're correct, however with the new changes it shouldn't be a problem either way, you can take a look :)

Copy link
Member

@swallez swallez left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for having caught this and the fix!

@l-trotta l-trotta merged commit 0c14348 into elastic:main Sep 12, 2024
4 checks passed
l-trotta added a commit that referenced this pull request Sep 12, 2024
* fix: bulk ingester might skip lister requests

* minor: fix style

* always waiting for listener to be done before closing

---------

Co-authored-by: Laura Trotta <[email protected]>
Co-authored-by: Laura Trotta <[email protected]>
l-trotta added a commit that referenced this pull request Sep 12, 2024
* fix: bulk ingester might skip lister requests

* minor: fix style

* always waiting for listener to be done before closing

---------

Co-authored-by: Laura Trotta <[email protected]>
Co-authored-by: Laura Trotta <[email protected]>
@fabriziofortino fabriziofortino deleted the bulk-ingester-fix branch September 12, 2024 16:14
l-trotta added a commit that referenced this pull request Sep 13, 2024
* fix: bulk ingester might skip lister requests

* minor: fix style

* always waiting for listener to be done before closing

---------

Co-authored-by: Laura Trotta <[email protected]>
Co-authored-by: Laura Trotta <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants