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

Engine: refresh before translog commit #13414

Merged
merged 1 commit into from Sep 9, 2015

Conversation

@brwe
Copy link
Contributor

commented Sep 9, 2015

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

@s1monw
s1monw reviewed Sep 9, 2015
View changes
core/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java Outdated
ParsedDocument doc = testParsedDocument("1", "1", "test", null, -1, -1, testDocumentWithTextField(), B_1, null);
engine.create(new Engine.Create(newUid("1"), doc));
AtomicReference<Engine.GetResult> latestGetResult = new AtomicReference<>();
AtomicBoolean flushFinished = new AtomicBoolean(false);

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 9, 2015

Contributor

these must be final for backporting?

@s1monw
s1monw reviewed Sep 9, 2015
View changes
core/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java Outdated
if (previousGetResult != null) {
previousGetResult.release();
}
try {

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 9, 2015

Contributor

can the sleep go?

This comment has been minimized.

Copy link
@brwe

brwe Sep 9, 2015

Author Contributor

yes

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2015

left minor commetns LGTM

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 brwe force-pushed the brwe:commit-refresh-order branch to 0ce66b4 Sep 9, 2015
@brwe

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2015

@s1monw thanks! addressed all comments

@mikemccand

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2015

Is there still a race here? I.e. after refresh but before translog.commit, another thread indexes some documents, but these documents are not visible via realtime get until the next refresh?

@brwe

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2015

@mikemccand good point. but if I understand it correctly we actually create a new translog before we refresh in translog.prepareCommit() and then in translog.commit() just delete the old one. Documents that came in between refresh() and translog.commit() will be in the new translog and therefore are still available from there after commit. Only docs that were in translog before prepareCommit() are not available from translog anymore but they can be retrieved from index. Let me know if this makes sense, I might just miss something.

@mikemccand

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2015

but if I understand it correctly we actually create a new translog before we refresh in translog.prepareCommit()

Aha! Sorry, I missed the prepareCommit call ... so you are correct! Thanks :)

@mikemccand

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2015

LGTM

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2015

LGTM 2

brwe added a commit that referenced this pull request Sep 9, 2015
Engine: refresh before translog commit
@brwe brwe merged commit fa9696f into elastic:master Sep 9, 2015
1 check passed
1 check passed
CLA Commit author is a member of Elasticsearch
Details
@clintongormley clintongormley added v2.0.0-beta2 and removed v2.0.0 labels Sep 14, 2015
@clintongormley clintongormley removed the v2.1.0 label Nov 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.