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

Make peer recovery work with archive data #81522

Merged
merged 12 commits into from Dec 14, 2021

Conversation

ywelsch
Copy link
Contributor

@ywelsch ywelsch commented Dec 8, 2021

Adapts peer recovery so that it properly integrates with the hook to convert old indices.

Relates #81210

@@ -169,6 +170,10 @@ public void write(Directory directory, SegmentInfo segmentInfo, String segmentSu
private static FieldInfos filterFields(FieldInfos fieldInfos) {
List<FieldInfo> fieldInfoCopy = new ArrayList<>(fieldInfos.size());
for (FieldInfo fieldInfo : fieldInfos) {
// omit sequence number field so that it doesn't interfere with peer recovery
if (fieldInfo.name.equals(SeqNoFieldMapper.NAME)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this, LuceneChangesSnapshot will just return empty set of documents instead of failing because the field has docvalues mapped to none now.

In a follow-up, I want to explore exposing doc-values of older indices (in particular _seq_no field and soft-deletes)

@@ -93,26 +86,6 @@ private static void ensureSnapshotIsLoaded(IndexShard indexShard) {
: "loading snapshot must not be called twice unless we are retrying a peer recovery";
}

private static void associateNewEmptyTranslogWithIndex(IndexShard indexShard) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is now gone. Woop woop

@ywelsch ywelsch added :Distributed/Recovery Anything around constructing a new shard, either from a local or a remote source. >non-issue labels Dec 8, 2021
@ywelsch ywelsch marked this pull request as ready for review December 8, 2021 16:11
@elasticmachine elasticmachine added the Team:Distributed Meta label for distributed team label Dec 8, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

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

LGTM - I left a comment

@ywelsch
Copy link
Contributor Author

ywelsch commented Dec 13, 2021

Unfortunately BWC tests were disabled on master when I branched my PR off, so these only ran now when I merged in latest master - AND FAILED. I had to take a different approach to hooking into the recovery logic (as I have to cover the case where the peer recovery source is on an old version).

The good news is that the hook is simpler than the one we had before. Sorry for the inconvenience of having to review this again.

@ywelsch ywelsch requested review from dnhatn and tlrx December 13, 2021 16:53
final Store store = indexShard.store();
store.incRef();
try {
StoreRecovery.bootstrap(indexShard, store);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this new approach, we now have sequence-number based recoveries for searchable snapshots, which means that there's no need any longer to send over any file (i.e. the exception of ^recovery\..*\.segments_.*$ in InMemoryNoOpCommitDirectory.ensureMutable) except for the BWC case (when primary was on older node and created a different history uuid).

Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

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

LGTM. I like the new iteration. Thanks @ywelsch.

Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

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

LGTM

@ywelsch ywelsch merged commit 1e99bc6 into elastic:master Dec 14, 2021
@ywelsch
Copy link
Contributor Author

ywelsch commented Dec 14, 2021

Thank you @dnhatn and @tlrx!

jdconrad pushed a commit to jdconrad/elasticsearch that referenced this pull request Dec 14, 2021
Adapts peer recovery so that it properly integrates with the hook to convert old indices.

Relates elastic#81210
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Recovery Anything around constructing a new shard, either from a local or a remote source. >non-issue Team:Distributed Meta label for distributed team v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants