-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Fix BlobStoreRepositoryCleanupIT Leaking Futures #69446
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
Conversation
The problem with elastic#69434 was that during master failover the listener might get retried after the new master has already dropped the cleanup from the cluster state which then conflicts with the cleanup executed by the repo consistency checks after each test. To prevent running into this conflict, just wait for the future to actually return. Closes elastic#69434
Pinging @elastic/es-distributed (Team:Distributed) |
@@ -80,13 +90,16 @@ private String startBlockedCleanup(String repoName) throws Exception { | |||
blockMasterFromFinalizingSnapshotOnIndexFile(repoName); | |||
|
|||
logger.info("--> starting repository cleanup"); | |||
client().admin().cluster().prepareCleanupRepository(repoName).execute(); | |||
// not running from a non-master client because shutting down a master while a request to it is pending might result in the future |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/not running/running/ ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
try { | ||
cleanupFuture.get(); | ||
} catch (ExecutionException e) { | ||
// ignored and expected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we be more specific about the exception that we expect here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea definitely, sorry for the laziness, made it a proper specific assertion now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem! :)
Thanks @fcofdez all fixed now I think :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Thanks Francisco! |
The problem with elastic#69434 was that during master failover the listener might get retried after the new master has already dropped the cleanup from the cluster state which then conflicts with the cleanup executed by the repo consistency checks after each test. To prevent running into this conflict, just wait for the future to actually return. Closes elastic#69434
The problem with elastic#69434 was that during master failover the listener might get retried after the new master has already dropped the cleanup from the cluster state which then conflicts with the cleanup executed by the repo consistency checks after each test. To prevent running into this conflict, just wait for the future to actually return. Closes elastic#69434
The problem with #69434 was that during master failover the listener might get retried after the new master has already dropped the cleanup from the cluster state which then conflicts with the cleanup executed by the repo consistency checks after each test. To prevent running into this conflict, just wait for the future to actually return. Closes #69434
The problem with #69434 was that during master failover the listener might get retried after the new master has already dropped the cleanup from the cluster state which then conflicts with the cleanup executed by the repo consistency checks after each test. To prevent running into this conflict, just wait for the future to actually return. Closes #69434
The problem with #69434 was that during master failover
the listener might get retried after the new master has
already dropped the cleanup from the cluster state which
then conflicts with the cleanup executed by the repo consistency
checks after each test.
To prevent running into this conflict, just wait for the future
to actually return.
Closes #69434