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

Return a CompletableFuture from BulkIngester.flush method #806

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aidin36
Copy link

@aidin36 aidin36 commented May 10, 2024

The Issue

In one case in our production app, we want to flush the bulk operations and make them immediately available for search. So, we do a flush on our BulkIngester instance, then we do a client.indices().refresh(). However, because the flush operation is asynchronous, most of the time before all the operations flush, the refresh gets called. So, some of the changes aren't immediately available for search.

The Solution

I returned a CompletableFuture from the flush method, so the user can wait for the flush to finish if required.

Documents

Sorry, I couldn't find where's the document for the flush method so I could update it. If you point me to it, I can update the return type there.

gradlew check

There are nine failing tests. But they were failing before my changes too.

Copy link

cla-checker-service bot commented May 10, 2024

💚 CLA has been signed

@aidin36 aidin36 force-pushed the future-return-bulk-ingester-flush branch from 7216ae1 to e765121 Compare May 13, 2024 02:16
@l-trotta
Copy link
Contributor

Hello! Before changing the code, have you tried the refresh option in the globalSettings of the BulkIngester? Setting it to true would refresh after every BulkRequest , making the new data available to search.

@aidin36
Copy link
Author

aidin36 commented Jun 25, 2024

Hi (:
Thanks for the suggestion. Ideally, I don't want to refresh on all the bulk requests and let Elastic Search decide when is the right time to do so. We only need it in a specific case.

Besides my use case, I believe it's a good idea to return a Future from all the async methods and allow the client to know what happened to the result of the call.

@sebastianrtb
Copy link

Hi,

I also think that this change is really necessary as it is resolving also our use case as well. For us it is important that flush will wait untill afterBulk will be called, so with this change we would simply wait for flush to finish.

@fabriziofortino
Copy link
Contributor

I think this relates to #703

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.

4 participants