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

Integrate translog recovery into Engine / InternalEngine #10452

Merged
merged 1 commit into from Apr 13, 2015

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented Apr 7, 2015

Today the engine writes the transaction log itself as well as manages
all the commit / translog mapping internally. Yet, if an engine is closed
and reopend it doesn't replay it's translog or does anything to be consistent
with it's latest state again.
This change moves the transaction log replay code into the Engine / InternalEngine
and adds unittests for replaying and consistency.

@s1monw
Copy link
Contributor Author

s1monw commented Apr 7, 2015

@bleskes if you have a day or two to review ;)

@s1monw s1monw added the blocker label Apr 7, 2015
/**
* Returns <code>true</code> if initial translog recovery should be skipped. Otherwise <code>false</code>
*/
public boolean isSkipInitialRecovery() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the boolean pattern uses "is" but I think this would make more sense named shouldSkipInitialRecovery?

@s1monw s1monw force-pushed the recover_from_translog_in_engine branch 3 times, most recently from 5150dbd to 4517a95 Compare April 10, 2015 10:03
@@ -1051,6 +1060,69 @@ public void flushAndClose() throws IOException {
}
}

protected TranslogRecoveryStats recoverFromTranslog(long translogId, TranslogRecoveryPerformer handler) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I this should go into the InternalEngine. The shadow engine and a future read only engine don't need this, right?

}
final MockDirectoryWrapper directory = DirectoryUtils.getLeaf(store.directory(), MockDirectoryWrapper.class);
if(directory != null) {
directory.setPreventDoubleWrite(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wondering, why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because es is broken

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I replied more here... sorry it buggs me a lot that we have to do that... since we rollback we are writing the same segment files again after starting indexwirter but mockDirWrapper prevents this so we have to disable...I will add a comment

@bleskes
Copy link
Contributor

bleskes commented Apr 13, 2015

I like the change. Left a bunch of comments. Most of them minor, with the exception of the recovery state stats handling.

@s1monw
Copy link
Contributor Author

s1monw commented Apr 13, 2015

pushed some updates

@@ -620,7 +625,8 @@ protected int sendSnapshot(Translog.Snapshot snapshot) throws ElasticsearchExcep
if (ops >= recoverySettings.translogOps() || size >= recoverySettings.translogSize().bytes()) {

// don't throttle translog, since we lock for phase3 indexing,
// so we need to move it as fast as possible. Note, since we
// so we need to move it as fast as possible. Note,
// ince we
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

accidental enter?

@s1monw
Copy link
Contributor Author

s1monw commented Apr 13, 2015

@bleskes pushed a new commit

@s1monw
Copy link
Contributor Author

s1monw commented Apr 13, 2015

@bleskes I pushed that stuff into the performer as you requested I hope it's worth all the added complexity

@bleskes
Copy link
Contributor

bleskes commented Apr 13, 2015

LGTM. Thx @s1monw

@s1monw s1monw force-pushed the recover_from_translog_in_engine branch from 074939c to 05ae57c Compare April 13, 2015 14:41
s1monw added a commit to s1monw/elasticsearch that referenced this pull request Apr 13, 2015
Today the engine writes the transaction log itself as well as manages
all the commit / translog mapping internally. Yet, if an engine is closed
and reopend it doesn't replay it's translog or does anything to be consistent
with it's latest state again.
This change moves the transaction log replay code into the Engine / InternalEngine
and adds unittests for replaying and consistency.

Closes elastic#10452
Today the engine writes the transaction log itself as well as manages
all the commit / translog mapping internally. Yet, if an engine is closed
and reopend it doesn't replay it's translog or does anything to be consistent
with it's latest state again.
This change moves the transaction log replay code into the Engine / InternalEngine
and adds unittests for replaying and consistency.

Closes elastic#10452
@s1monw s1monw force-pushed the recover_from_translog_in_engine branch from 05ae57c to b1c9dfc Compare April 13, 2015 14:42
@s1monw s1monw merged commit b1c9dfc into elastic:master Apr 13, 2015
@s1monw s1monw deleted the recover_from_translog_in_engine branch April 14, 2015 14:40
@clintongormley clintongormley changed the title [RECOVERY] Integrate translog recovery into Engine / InternalEngine Integrate translog recovery into Engine / InternalEngine Jun 7, 2015
@clintongormley clintongormley added :Distributed/Recovery Anything around constructing a new shard, either from a local or a remote source. and removed :Translog labels Jun 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker :Distributed/Recovery Anything around constructing a new shard, either from a local or a remote source. >enhancement v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants