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

Bulk processor#awaitClose to close scheduler #29263

Merged
merged 2 commits into from Mar 28, 2018

Conversation

Projects
None yet
5 participants
@javanna
Copy link
Member

commented Mar 27, 2018

When the BulkProcessor is used with the high-level REST client, a scheduler is internally created that allows to schedule tasks. Such scheduler is not exposed to users and needs to be closed once the BulkProcessor is closed. There are two ways to close the BulkProcessor though, one is the ordinary close method and the other one is awaitClose. The former closes the scheduler while the latter doesn't, leaving threads lingering.

I discovered this when adding some tests for the bulk processor, as we mainly test it with the transport client, and the same tests run with the high-level REST client already uncovered a bug.

Bulk processor#awaitClose to close scheduler
When the `BulkProcessor` is used with the high-level REST client, a scheduler is internally created that allows to schedule tasks. Such scheduler is not exposed to users and needs to be closed once the `BulkProcessor` is closed. There are two ways to close the `BulkProcessor` though, one is the ordinary `close` method and the other one is `awaitClose`. The former closes the scheduler while the latter doesn't, leaving threads lingering.

I discovered this when adding some tests for the bulk processor, as we mainly test it with the transport client, and the same tests run with the high-level REST client already uncovered a bug.
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Mar 27, 2018

@nik9000
Copy link
Contributor

left a comment

LGTM

@tlrx
Copy link
Member

left a comment

This looks great, thanks a lot for all the tests. I added minor comments.

Response response = client().performRequest("PUT", "/test-ro", Collections.emptyMap(), entity);
assertThat(response.getStatusLine().getStatusCode(), equalTo(200));

//ensureGreen();

This comment has been minimized.

Copy link
@tlrx

tlrx Mar 28, 2018

Member

nit: can you remove this?

try (BulkProcessor processor = initBulkProcessorBuilder(listener)
.setConcurrentRequests(concurrentRequests).setBulkActions(bulkActions)
//set interval and size to high values
.setFlushInterval(TimeValue.timeValueHours(24)).setBulkSize(new ByteSizeValue(1, ByteSizeUnit.GB)).build()) {

This comment has been minimized.

Copy link
@tlrx

tlrx Mar 28, 2018

Member

Looks like we always use these values, maybe we could set them in initBulkProcessorBuilder?

This comment has been minimized.

Copy link
@javanna

javanna Mar 28, 2018

Author Member

we almost always use the same values, but not exactly the same, there are subtle differences. I took these tests from the existing BulkProcessorIT tests, and adapted them from transport client to rest high level client. The idea was to not touch anything and see if things still work the same way, which they kind of do (up to bugs)

This comment has been minimized.

Copy link
@tlrx

tlrx Mar 28, 2018

Member

Okay

@@ -237,7 +236,9 @@ public synchronized boolean awaitClose(long timeout, TimeUnit unit) throws Inter
if (bulkRequest.numberOfActions() > 0) {
execute();
}
return this.bulkRequestHandler.awaitClose(timeout, unit);
boolean awaitClose = this.bulkRequestHandler.awaitClose(timeout, unit);
onClose.run();

This comment has been minimized.

Copy link
@tlrx

tlrx Mar 28, 2018

Member

I'm wondering if we should ensure that the onClose.run() is always executed (like in a finally block) if something goes wrong in bulkRequestHandler.awaitClose(). I don't have a strong feeling about this, and if we decide to add it we should also have a test for it, but I think it would be safer.

@tlrx

tlrx approved these changes Mar 28, 2018

Copy link
Member

left a comment

LGTM

@javanna javanna merged commit 245dd73 into elastic:master Mar 28, 2018

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details

javanna added a commit that referenced this pull request Mar 28, 2018

Bulk processor#awaitClose to close scheduler (#29263)
When the `BulkProcessor` is used with the high-level REST client, a scheduler is internally created that allows to schedule tasks. Such scheduler is not exposed to users and needs to be closed once the `BulkProcessor` is closed. There are two ways to close the `BulkProcessor` though, one is the ordinary `close` method and the other one is `awaitClose`. The former closes the scheduler while the latter doesn't, leaving threads lingering.

javanna added a commit that referenced this pull request Mar 28, 2018

Bulk processor#awaitClose to close scheduler (#29263)
When the `BulkProcessor` is used with the high-level REST client, a scheduler is internally created that allows to schedule tasks. Such scheduler is not exposed to users and needs to be closed once the `BulkProcessor` is closed. There are two ways to close the `BulkProcessor` though, one is the ordinary `close` method and the other one is `awaitClose`. The former closes the scheduler while the latter doesn't, leaving threads lingering.

@colings86 colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.