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

Store Pending Deletions Fix #40345

Conversation

Projects
None yet
6 participants
@henningandersen
Copy link
Contributor

commented Mar 22, 2019

FilterDirectory.getPendingDeletions does not delegate, fixed
temporarily by overriding in StoreDirectory.

This in turn caused duplicate file name use after a trimUnsafeCommits
had been done, since a new IndexWriter would not consider the pending
deletes in IndexFileDeleter. This should only happen on windows (AFAIK).

Reenabled doing index updates for all tests using
IndexShardTests.indexOnReplicaWithGaps (which could fail due to above
when using mocked WindowsFS).

Follow-up to #40249

A follow-up should fix this in FilterDirectory.

Store Pending Files Fix
FilterDirectory.getPendingDeletions does not delegate, fixed
temporarily by overriding in StoreDirectory.

This in turn caused duplicate file name use after a trimUnsafeCommits
had been done, since a new IndexWriter would not consider the pending
deletes in IndexFileDeleter. This should only happen on windows (AFAIK).

Reenabled doing index updates for all tests using
IndexShardTests.indexOnReplicaWithGaps (which could fail due to above
when using mocked WindowsFS).
@elasticmachine

This comment has been minimized.

Copy link

commented Mar 22, 2019

@henningandersen henningandersen requested a review from ywelsch Mar 22, 2019

@henningandersen henningandersen changed the title Store Pending Files Fix Store Pending Deletions Fix Mar 22, 2019

@henningandersen henningandersen removed the v6.6.3 label Mar 22, 2019

@ywelsch
Copy link
Contributor

left a comment

Great catch! LGTM

@dnhatn

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

@henningandersen I also dug into a recent test failure of testRestoreLocalHistoryFromTranslogOnPromotion (https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+master+multijob-unix-compatibility/os=centos-6/312/console). I think that test failure has the same cause as the one you are fixing here. I proposed https://issues.apache.org/jira/browse/LUCENE-8734 to record nextWriteDVGen.

@ywelsch ywelsch requested review from s1monw and dnhatn Mar 22, 2019

@henningandersen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

@elasticmachine run elasticsearch-ci/packaging-sample

public Set<String> getPendingDeletions() throws IOException {
// FilterDirectory.getPendingDeletions does not delegate, working around it here.
// to be removed once fixed in FilterDirectory.
return unwrap(this).getPendingDeletions();

This comment has been minimized.

Copy link
@s1monw

s1monw Mar 22, 2019

Contributor

I think this fix is trappy, but I am ok with it for now. I would like us to delegate all our subclasses of FilterDirectory as well. We have a few more. I am torn on the unwrap to be honest.

This comment has been minimized.

Copy link
@henningandersen

henningandersen Mar 22, 2019

Author Contributor

The problem with that route is that most of the subclasses of this are in lucene, including the MockDirectoryWrapper created by LuceneTestCase and for instance HardlinkCopyDirectoryWrapper used by StoreRecovery.
I agree that using unwrap is questionable, but in the given context this should be safe here until fixed in lucene.

This comment has been minimized.

Copy link
@s1monw

s1monw Mar 26, 2019

Contributor

I agree, we should still pro-actively implement the right fix in all subclasses and remove the unwrap once lucene is upgraded

This comment has been minimized.

Copy link
@henningandersen

henningandersen Mar 26, 2019

Author Contributor

Thanks @s1monw for clarifying. Except for two trivial test-only subclasses, I have added the override to all subclasses in 85f2066.

Please have another look at your convenience.

@henningandersen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

@elasticmachine run elasticsearch-ci/packaging-sample

1 similar comment
@henningandersen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

@elasticmachine run elasticsearch-ci/packaging-sample

@dnhatn

dnhatn approved these changes Mar 24, 2019

Copy link
Contributor

left a comment

Great find!

henningandersen added some commits Mar 26, 2019

Store Pending Files Fix
Added getPendingDeletions delegation to all elasticsearch
FilterDirectory subclasses that were not trivial test-only overrides to
minimize the risk of hitting this issue in another case.
@s1monw

s1monw approved these changes Mar 26, 2019

@henningandersen henningandersen merged commit cca77c1 into elastic:master Mar 26, 2019

8 checks passed

CLA All commits in pull request signed
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/bwc Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

henningandersen added a commit that referenced this pull request Mar 26, 2019

Store Pending Deletions Fix (#40345)
FilterDirectory.getPendingDeletions does not delegate, fixed
temporarily by overriding in StoreDirectory.

This in turn caused duplicate file name use after a trimUnsafeCommits
had been done, since a new IndexWriter would not consider the pending
deletes in IndexFileDeleter. This should only happen on windows (AFAIK).

Reenabled doing index updates for all tests using
IndexShardTests.indexOnReplicaWithGaps (which could fail due to above
when using mocked WindowsFS).

Added getPendingDeletions delegation to all elasticsearch
FilterDirectory subclasses that were not trivial test-only overrides to
minimize the risk of hitting this issue in another case.

henningandersen added a commit that referenced this pull request Mar 26, 2019

Store Pending Deletions Fix (#40345)
FilterDirectory.getPendingDeletions does not delegate, fixed
temporarily by overriding in StoreDirectory.

This in turn caused duplicate file name use after a trimUnsafeCommits
had been done, since a new IndexWriter would not consider the pending
deletes in IndexFileDeleter. This should only happen on windows (AFAIK).

Reenabled doing index updates for all tests using
IndexShardTests.indexOnReplicaWithGaps (which could fail due to above
when using mocked WindowsFS).

Added getPendingDeletions delegation to all elasticsearch
FilterDirectory subclasses that were not trivial test-only overrides to
minimize the risk of hitting this issue in another case.

henningandersen added a commit that referenced this pull request Mar 26, 2019

Store Pending Deletions Fix (#40345)
FilterDirectory.getPendingDeletions does not delegate, fixed
temporarily by overriding in StoreDirectory.

This in turn caused duplicate file name use after a trimUnsafeCommits
had been done, since a new IndexWriter would not consider the pending
deletes in IndexFileDeleter. This should only happen on windows (AFAIK).

Reenabled doing index updates for all tests using
IndexShardTests.indexOnReplicaWithGaps (which could fail due to above
when using mocked WindowsFS).

Added getPendingDeletions delegation to all elasticsearch
FilterDirectory subclasses that were not trivial test-only overrides to
minimize the risk of hitting this issue in another case.

henningandersen added a commit that referenced this pull request Mar 26, 2019

Store Pending Deletions Fix (#40345)
Added missing file/commit.

@jakelandis jakelandis added v7.0.0-rc2 and removed v7.0.0 labels Apr 3, 2019

henningandersen added a commit to henningandersen/elasticsearch that referenced this pull request Apr 16, 2019

Remove pending deletions workaround
The recent upgrade to lucene 8.1.0 included the fix for LUCENE-8735
(pending deletions did not delegate). Removed the temporary workaround
for this.

Follow up to elastic#40345

henningandersen added a commit that referenced this pull request Apr 17, 2019

Remove pending deletions workaround (#41245)
The recent upgrade to lucene 8.1.0 included the fix for LUCENE-8735
(pending deletions did not delegate). Removed the temporary workaround
that was previously added for this issue.

Follow up to #40345

gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019

Remove pending deletions workaround (elastic#41245)
The recent upgrade to lucene 8.1.0 included the fix for LUCENE-8735
(pending deletions did not delegate). Removed the temporary workaround
that was previously added for this issue.

Follow up to elastic#40345
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.