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

Incremental restores treat segments containing soft-deleted docs as "different" #55142

Closed
DaveCTurner opened this issue Apr 14, 2020 · 2 comments · Fixed by #77695
Closed

Incremental restores treat segments containing soft-deleted docs as "different" #55142

DaveCTurner opened this issue Apr 14, 2020 · 2 comments · Fixed by #77695
Assignees
@DaveCTurner
Labels
:Distributed Indexing/Store Issues around managing unopened Lucene indices. If it touches Store.java, this is a likely label. Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination.

Comments

@DaveCTurner
Copy link
Contributor

DaveCTurner commented Apr 14, 2020 •
edited
Loading

Today in restores and peer recoveries we treat any .liv files as per-commit rather than per-segment since they may change independently of the rest of the segment as documents are deleted:

elasticsearch/server/src/main/java/org/elasticsearch/index/store/Store.java

Lines 963 to 969 in a862dff

if (IndexFileNames.SEGMENTS.equals(segmentId) ||
DEL_FILE_EXTENSION.equals(extension) || LIV_FILE_EXTENSION.equals(extension)) {
// only treat del files as per-commit files fnm files are generational but only for upgradable DV
perCommitStoreFiles.add(meta);
} else {
perSegment.computeIfAbsent(segmentId, k -> new ArrayList<>()).add(meta);
}

This means that a restore or peer recovery that only deletes documents need only copy the corresponding .liv files. However today we soft-delete documents instead which updates their doc values, not their liveness markers, which means we fully restore/recover any segments whose set of deleted documents have changed.

Relates #55013
Relates https://discuss.elastic.co/t/227184
Relates https://issues.apache.org/jira/browse/LUCENE-9324
@DaveCTurner DaveCTurner added the :Distributed Indexing/Store Issues around managing unopened Lucene indices. If it touches Store.java, this is a likely label. label Apr 14, 2020
@DaveCTurner DaveCTurner self-assigned this Apr 14, 2020
@elasticmachine
Copy link
Collaborator

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

@ywelsch ywelsch mentioned this issue Apr 14, 2020
Merged
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this issue Apr 15, 2020
@DaveCTurner
Respect generational files in recoveryDiff
6930e8b 
Today `MetadataSnapshot#recoveryDiff` considers the `.liv` file as per-commit
rather than per-segment and often transfers them during peer recoveries and
snapshot restores. It also considers differences in `.fnm`, `.dvd` and `.dvm`
files as indicating a difference in the whole segment, even though these files
may be adjusted without changing the segment itself.

This commit adjusts this logic to attach these generational files to the
segments themselves, allowing Elasticsearch only to transfer them if they are
genuinely needed.

Closes elastic#55142
Resolves an outstanding `//NORELEASE` action related to elastic#50999.
@DaveCTurner DaveCTurner mentioned this issue Apr 15, 2020
Closed
@rjernst rjernst added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label May 4, 2020
@ywelsch ywelsch mentioned this issue Jul 1, 2020
Merged
@yjchen-tw
Copy link

any update for this issue?
it seems to have a PR #55239 to fix this but have been postponed from v7.9.0 to v7.14.0
we suffer from large network traffic during restore due to newly deleted documents in large segments
would be happy to know if this can be fixed in recent release

tlrx added a commit to tlrx/elasticsearch that referenced this issue May 18, 2021
@tlrx
[7.x] Fix SearchableSnapshotsPersistentCacheIntegTests.testPersistent…
e4713d5 
…CacheCleanUpAfterRelocation

On 7.x the test should always use soft deletes because
peer recovery always copies .liv files but searchable
snapshot directories do not support writing files.

Relates elastic#55142
Closes elastic#72857
@tlrx tlrx mentioned this issue May 18, 2021
Merged
tlrx added a commit that referenced this issue May 19, 2021
@tlrx
[7.x] Fix SearchableSnapshotsPersistentCacheIntegTests.testPersistent…
3627275 
…CacheCleanUpAfterRelocation (#73207)

On 7.x the test should always use soft deletes because
peer recovery always copies .liv files but searchable
snapshot directories do not support writing files.

Relates #55142
Closes #72857
@fcofdez fcofdez mentioned this issue Sep 14, 2021
Merged
fcofdez added a commit that referenced this issue Oct 5, 2021
@fcofdez @DaveCTurner
Respect generational files in recoveryDiff (#77695)
310b4ac 
Today `MetadataSnapshot#recoveryDiff` considers the `.liv` file as per-commit
rather than per-segment and often transfers them during peer recoveries and
snapshot restores. It also considers differences in `.fnm`, `.dvd` and `.dvm`
files as indicating a difference in the whole segment, even though these files
may be adjusted without changing the segment itself.

This commit adjusts this logic to attach these generational files to the
segments themselves, allowing Elasticsearch only to transfer them if they are
genuinely needed.

Closes #55142

Co-authored-by: David Turner <[email protected]>
fcofdez added a commit to fcofdez/elasticsearch that referenced this issue Oct 5, 2021
@fcofdez @DaveCTurner
Respect generational files in recoveryDiff
0e164f6 
Today `MetadataSnapshot#recoveryDiff` considers the `.liv` file as per-commit
rather than per-segment and often transfers them during peer recoveries and
snapshot restores. It also considers differences in `.fnm`, `.dvd` and `.dvm`
files as indicating a difference in the whole segment, even though these files
may be adjusted without changing the segment itself.

This commit adjusts this logic to attach these generational files to the
segments themselves, allowing Elasticsearch only to transfer them if they are
genuinely needed.

Closes elastic#55142
Backport of elastic#77695

Co-authored-by: David Turner <[email protected]>
@fcofdez fcofdez mentioned this issue Oct 5, 2021
Merged
fcofdez added a commit that referenced this issue Oct 6, 2021
@fcofdez @DaveCTurner
Respect generational files in recoveryDiff (#78707)
97d5a75 
Today `MetadataSnapshot#recoveryDiff` considers the `.liv` file as per-commit
rather than per-segment and often transfers them during peer recoveries and
snapshot restores. It also considers differences in `.fnm`, `.dvd` and `.dvm`
files as indicating a difference in the whole segment, even though these files
may be adjusted without changing the segment itself.

This commit adjusts this logic to attach these generational files to the
segments themselves, allowing Elasticsearch only to transfer them if they are
genuinely needed.

Closes #55142
Backport of #77695

Co-authored-by: David Turner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@DaveCTurner DaveCTurner

Labels
:Distributed Indexing/Store Issues around managing unopened Lucene indices. If it touches Store.java, this is a likely label. Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination.
Projects
None yet
Milestone
No milestone
4 participants
@rjernst @DaveCTurner @yjchen-tw @elasticmachine