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

Do not wrap soft-deletes reader for segment stats #51331

Merged
merged 1 commit into from
Jan 23, 2020

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Jan 22, 2020

IndexWriter might not filter out fully deleted segments if retention leases exist or the number of the retaining operations is non-zero. SoftDeletesDirectoryReaderWrapper, however, always filters out fully deleted segments.

This test failed because we randomly set index.soft_deletes.retention.operations.

This change uses the original directory reader when calculating segment stats instead.

Relates #51192
Closes #51303

@dnhatn dnhatn added >non-issue :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. v8.0.0 v6.8.7 v7.7.0 v7.6.1 v7.5.3 labels Jan 22, 2020
@dnhatn dnhatn requested review from jpountz, tlrx and ywelsch January 22, 2020 19:37
@elasticmachine
Copy link
Collaborator

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

@dnhatn dnhatn added >bug and removed >non-issue labels Jan 22, 2020
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.

Thanks Nhat! This LGTM, but it's better to wait for another pair of eyes before merging.

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

LGTM

@dnhatn
Copy link
Member Author

dnhatn commented Jan 23, 2020

@tlrx @ywelsch Thanks for reviewing.

@dnhatn dnhatn merged commit 5132715 into elastic:master Jan 23, 2020
@dnhatn dnhatn deleted the fix-segment-stats branch January 23, 2020 13:26
dnhatn added a commit that referenced this pull request Jan 23, 2020
IndexWriter might not filter out fully deleted segments if retention
leases exist or the number of the retaining operations is non-zero.
SoftDeletesDirectoryReaderWrapper, however, always filters out fully
deleted segments.

This change uses the original directory reader when calculating segment
stats instead.

Relates #51192
Closes #51303
dnhatn added a commit that referenced this pull request Jan 23, 2020
IndexWriter might not filter out fully deleted segments if retention
leases exist or the number of the retaining operations is non-zero.
SoftDeletesDirectoryReaderWrapper, however, always filters out fully
deleted segments.

This change uses the original directory reader when calculating segment
stats instead.

Relates #51192
Closes #51303
dnhatn added a commit that referenced this pull request Jan 23, 2020
IndexWriter might not filter out fully deleted segments if retention
leases exist or the number of the retaining operations is non-zero.
SoftDeletesDirectoryReaderWrapper, however, always filters out fully
deleted segments.

This change uses the original directory reader when calculating segment
stats instead.

Relates #51192
Closes #51303
@polyfractal polyfractal added v7.6.0 and removed v7.6.1 labels Feb 7, 2020
@tlrx tlrx mentioned this pull request Mar 9, 2020
dnhatn added a commit that referenced this pull request Mar 10, 2020
We can't always have the same segment stats and doc stats between 
InternalEngine and ReadOnlyEngine if there are some fully deleted 
segments. ReadOnlyEngine always filters out them. InternalEngine,
however, will keep them if peer recovery retention leases exist or the
number of the retaining operations is non-zero.

This change reverts the fix in #51331 and uses the wrapped reader to 
calculate the segment stats and doc stats. For the test, we need to
disable the extra retaining soft-deletes operations.

Closes #51303
dnhatn added a commit that referenced this pull request Mar 11, 2020
We can't always have the same segment stats and doc stats between
InternalEngine and ReadOnlyEngine if there are some fully deleted
segments. ReadOnlyEngine always filters out them. InternalEngine,
however, will keep them if peer recovery retention leases exist or the
number of the retaining operations is non-zero.

This change reverts the fix in #51331 and uses the wrapped reader to
calculate the segment stats and doc stats. For the test, we need to
disable the extra retaining soft-deletes operations.

Closes #51303
dnhatn added a commit that referenced this pull request Mar 11, 2020
We can't always have the same segment stats and doc stats between
InternalEngine and ReadOnlyEngine if there are some fully deleted
segments. ReadOnlyEngine always filters out them. InternalEngine,
however, will keep them if peer recovery retention leases exist or the
number of the retaining operations is non-zero.

This change reverts the fix in #51331 and uses the wrapped reader to
calculate the segment stats and doc stats. For the test, we need to
disable the extra retaining soft-deletes operations.

Closes #51303
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. v7.5.3 v7.6.0 v7.7.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NoOpEngineTests failure
6 participants