Skip to content

Conversation

@bleskes
Copy link
Contributor

@bleskes bleskes commented Apr 18, 2017

The maxUnsafeAutoIdTimestamp timestamp is a safety marker guaranteeing that no retried-indexing operation with a higher auto gen id timestamp was process by the engine. This allows us to safely process documents without checking if they were seen before.

Currently this property is maintained in memory and is handed off from the primary to any replica during the recovery process.

This PR takes a more natural approach and stores it in the lucene commit, using the same semantics (no retry op with a higher time stamp is part of this commit). This means that the knowledge is transferred during the file copy and also means that we don't need to worry about crazy situations where an original append only request arrives at the engine after a retry was processed and the engine was restarted.

Once this is in 5.x, I will submit a follow up PR to remove this part of the recovery logic.

The `maxUnsafeAutoIdTimestamp` time stamp is a safety marker guaranteeing that no indexing operation with
a higher auto gen id timestamp was process by the engine. This allows us to safely process documents
without checking if they were seen before.

Currently this property is maintained in memory and is handed off from the primary to any replica during
the recovery process.

This PR takes a more natural approach and stores it in the lucene commit, using the same semantics (no
op with a higher time stamp is part of this commit). This means that the knowledge is transferred during
the file copy and also means that we don't need to worry about crazy situations where an original append
only request arrives at the engine after a retry was processed *and* the engine was restarted.
@bleskes bleskes added :Engine :Distributed Indexing/Recovery Anything around constructing a new shard, either from a local or a remote source. >enhancement v5.5.0 v6.0.0-alpha1 labels Apr 18, 2017
@bleskes bleskes requested a review from s1monw April 18, 2017 07:43
@s1monw
Copy link
Contributor

s1monw commented Apr 18, 2017

Once this is in 5.x, I will submit a follow up PR to remove this part of the recovery logic.

question, this won't be in all 5.x indices since they might do a full cluster restart? So we can't remove it since we might need it during recover process? The other option is to detect this on open but then we need to do this for all 5.x indices I think

Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

I think it's a good change... left a suggestion

}

private void updateMaxUnsafeAutoIdTimestampFromWriter(IndexWriter writer) {
long commitMaxUnsafeAutoIdTimestamp = Long.MIN_VALUE;
Copy link
Contributor

Choose a reason for hiding this comment

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

what's wrong with writer.getLiveCommitData().getOrDefault(MAX_UNSAFE_AUTO_ID_TIMESTAMP_COMMIT_ID, Long.MIN_VALUE);?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's an iterable :

public final synchronized Iterable<Map.Entry<String,String>> getLiveCommitData() {

I can throw it into a HashMap, if you prefer?

Copy link
Member

Choose a reason for hiding this comment

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

I wondered if there was something better than iterating too but there's not since IndexWriter#getLiveCommitData only returns an Iterable.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh yeah oh well :) fair enough...

@bleskes
Copy link
Contributor Author

bleskes commented Apr 18, 2017

question, this won't be in all 5.x indices since they might do a full cluster restart?

Agree it won't be there. However, on full cluster restart the primary is already opened with the maxUnsafeAutoIdTimestamp set to -1 and it flushes on start. This means that the future replicas will receive it when they do a file sync, so I think we can still remove this from the peer recovery code?

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM.

@s1monw
Copy link
Contributor

s1monw commented Apr 18, 2017

Agree it won't be there. However, on full cluster restart the primary is already opened with the maxUnsafeAutoIdTimestamp set to -1 and it flushes on start. This means that the future replicas will receive it when they do a file sync, so I think we can still remove this from the peer recovery code?

fair enough - lets put a comment in the code pls

@bleskes bleskes merged commit edff30f into elastic:master Apr 18, 2017
@bleskes bleskes deleted the commit_max_autogentimestamp branch April 18, 2017 18:11
@bleskes
Copy link
Contributor Author

bleskes commented Apr 18, 2017

Thx @s1monw & @jasontedor

bleskes added a commit that referenced this pull request Apr 18, 2017
The `maxUnsafeAutoIdTimestamp` timestamp is a safety marker guaranteeing that no retried-indexing operation with a higher auto gen id timestamp was process by the engine. This allows us to safely process documents without checking if they were seen before.

Currently this property is maintained in memory and is handed off from the primary to any replica during the recovery process.

This commit takes a more natural approach and stores it in the lucene commit, using the same semantics (no retry op with a higher time stamp is part of this commit). This means that the knowledge is transferred during the file copy and also means that we don't need to worry about crazy situations where an original append only request arrives at the engine after a retry was processed *and* the engine was restarted.
bleskes added a commit that referenced this pull request Apr 21, 2017
With #24149 , it is now stored in the Lucene commit and is implicitly transferred in the file phase of the recovery.
asettouf pushed a commit to asettouf/elasticsearch that referenced this pull request Apr 23, 2017
With elastic#24149 , it is now stored in the Lucene commit and is implicitly transferred in the file phase of the recovery.
bleskes added a commit that referenced this pull request Sep 22, 2017
…ID if index was created before 5.5.0

It was adding in #24149 which was merged into 5.5.0
bleskes added a commit that referenced this pull request Sep 22, 2017
…ID if index was created before 5.5.0

It was adding in #24149 which was merged into 5.5.0
@clintongormley clintongormley added :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. and removed :Engine :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. labels Feb 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. :Distributed Indexing/Recovery Anything around constructing a new shard, either from a local or a remote source. >enhancement v5.5.0 v6.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants