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.

Sign up for GitHub

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

Jump to bottom

CI: BulkIT.testBulkUpdate_largerVolume fails one of its GetRequests #13379

Closed
spinscale opened this issue Sep 7, 2015 · 2 comments · Fixed by #13414
Closed

CI: BulkIT.testBulkUpdate_largerVolume fails one of its GetRequests #13379

spinscale opened this issue Sep 7, 2015 · 2 comments · Fixed by #13414
Assignees
@brwe
Labels
>bug >test Issues or PRs that are addressing/adding tests

Comments

@spinscale
Copy link
Contributor

http://build-us-00.elastic.co/job/es_core_2x_strong/28/

The issue is reproducible most of the time running this seed (on osx), further parameters having no influence already removed:

mvn verify -Pdev -Dskip.unit.tests -pl org.elasticsearch:elasticsearch -Dtests.seed=872FF07FDDFBCB -Dtests.class=org.elasticsearch.document.BulkIT -Dtests.method="testBulkUpdate_largerVolume" -Des.logger.level=DEBUG -Des.node.mode=local

When not running in local mode, this does not occur, also adding waitForRelocations makes it vanish, but I am not sure, if this is the underlying issue here.
@spinscale spinscale added the >test Issues or PRs that are addressing/adding tests label Sep 7, 2015
@brwe brwe self-assigned this Sep 7, 2015
@brwe
Copy link
Contributor

brwe commented Sep 8, 2015

I think this is a bug in InternalEngine.flush(). When we flush we call refresh() and translog.commit() in the wrong order here: https://github.com/elastic/elasticsearch/blob/master/core/src/main/java/org/elasticsearch/index/engine/InternalEngine.java#L759
This leaves a short gap in which we can't get a document from translog and index.
The test fails reliable for me but when I switch the order it passes.
I will work on a unit test and make a pr.

@brwe brwe added >bug v2.0.0 and removed v2.0.0 labels Sep 8, 2015
@s1monw
Copy link
Contributor

s1monw commented Sep 9, 2015

I think this is a bug in InternalEngine.flush(). When we flush we call refresh() and translog.commit() in the wrong order here:

whoa!!! good catch we need a get and commit stress test... that reproduces this problem in isolation

@brwe brwe mentioned this issue Sep 9, 2015
Merged
brwe added a commit to brwe/elasticsearch that referenced this issue Sep 9, 2015
@brwe
Engine: refresh before translog commit
0ce66b4 
When we commit the translog, documents that were in it before cannot be retrieved from
it anymore via get and have to be retrieved from the index instead. But they will only
be visible if between index and get a refresh is called. Therfore we have to call
first refresh and then translog.commit() because otherwise there is a small gap
in which we cannot read from the translog anymore but also not from the index.

closes elastic#13379
brwe added a commit that referenced this issue Sep 9, 2015
@brwe
Engine: refresh before translog commit
cbc32ea 
When we commit the translog, documents that were in it before cannot be retrieved from
it anymore via get and have to be retrieved from the index instead. But they will only
be visible if between index and get a refresh is called. Therfore we have to call
first refresh and then translog.commit() because otherwise there is a small gap
in which we cannot read from the translog anymore but also not from the index.

closes #13379
brwe added a commit that referenced this issue Sep 9, 2015
@brwe
Engine: refresh before translog commit
5c74cac 
When we commit the translog, documents that were in it before cannot be retrieved from
it anymore via get and have to be retrieved from the index instead. But they will only
be visible if between index and get a refresh is called. Therfore we have to call
first refresh and then translog.commit() because otherwise there is a small gap
in which we cannot read from the translog anymore but also not from the index.

closes #13379
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@brwe brwe

Labels
>bug >test Issues or PRs that are addressing/adding tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Engine: refresh before translog commit brwe/elasticsearch
3 participants
@spinscale @s1monw @brwe