Make sure afterBulk is always called in BulkProcessor #6495
Conversation
| @@ -309,13 +311,17 @@ public void onFailure(Throwable e) { | |||
| success = true; | |||
| } catch (InterruptedException e) { | |||
| Thread.interrupted(); | |||
| //this doesn't seem right, we effectively call afterBulk without having called beforeBulk? | |||
There was a problem hiding this comment.
I think you should also check if we called it though?
There was a problem hiding this comment.
I might be wrong...but InterruptedException can only be thrown in semaphore.acquire, which is before beforeBulk gets called. IMO we should just remove this line, that's why I added the comment...
There was a problem hiding this comment.
yea makes sense or we move the beforeBulk before the semaphore which might make more sense since we want to keep the critical section as small as possible?
There was a problem hiding this comment.
I like the idea of moving the beforeBulk up one line. This removes the need for the beforeCalled variable as well. Pushed a new commit.
|
added a small comment... otherwise lgtm |
|
Updated according to review, also added a commit to make sure we don't call |
|
LGTM |
| return multiGetRequestBuilder; | ||
| } | ||
|
|
||
| private static void checkResponseItems(List<BulkItemResponse> bulkItemResponses, String index, String type, int numDocs) { |
There was a problem hiding this comment.
maybe rename those methods to assert* as this is what they do... same with the next one
|
LGTM |
Also strenghtened BulkProcessorTests by adding randomizations to existing tests and new tests for concurrent requests and expcetions Closes elastic#5038
Moved listener.beforeBulk up one line, before sempahore.acquire. This also removes the need for the beforeCalled boolean variable.
Also remove needless test method parameters
|
Merged |
clintongormley
changed the title
Java API: Make sure afterBulk is always called in BulkProcessor.
Also strenghtened BulkProcessorTests by adding randomizations to existing tests and new tests for concurrent request.
Closes #5038