Skip to content

Fix ConcurrentSnapshotsIT.testMasterFailoverOnFinalizationLoop#89648

Merged
original-brownbear merged 1 commit intoelastic:mainfrom
original-brownbear:89625
Aug 26, 2022
Merged

Fix ConcurrentSnapshotsIT.testMasterFailoverOnFinalizationLoop#89648
original-brownbear merged 1 commit intoelastic:mainfrom
original-brownbear:89625

Conversation

@original-brownbear
Copy link
Copy Markdown
Contributor

This fixes a minor bug introduced in #89572. We need to take the listeners
for the snapshot out of the listeners map for the repo right away before
trying to run the next repo operation like we did prior to that change.
Otherwise we run the risk of a fatal exception failing all operations for
the repo like a no-longer-master exception did in #89625 even though
the snapshot completed successfully.

closes #89625

This fixes a minor bug introduced in #89572. We need to take the listeners
for the snapshot out of the listeners map for the repo right away before
trying to run the next repo operation like we did prior to that change.
Otherwise we run the risk of a fatal exception failing all operations for
the repo like a no-longer-master exception did in #89625 even though
the snapshot completed successfully.

closes #89625
@original-brownbear original-brownbear added >test Issues or PRs that are addressing/adding tests :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.5.0 labels Aug 26, 2022
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Distributed Meta label for distributed team. label Aug 26, 2022
}

@Override
public void onFailure(Exception e) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We have this pattern (a future that only succeeds but never fails) in a couple of spots now. I'll try to create a follow-up to dry this up, it's quite noisy doing this manually everywhere.

Copy link
Copy Markdown
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, thanks for fixing this

@original-brownbear
Copy link
Copy Markdown
Contributor Author

Thanks Tanguy!

@original-brownbear original-brownbear merged commit 1b8c650 into elastic:main Aug 26, 2022
@original-brownbear original-brownbear deleted the 89625 branch August 26, 2022 13:12
@original-brownbear original-brownbear restored the 89625 branch April 18, 2023 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs Team:Distributed Meta label for distributed team. >test Issues or PRs that are addressing/adding tests v8.5.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] ConcurrentSnapshotsIT testMasterFailoverOnFinalizationLoop failing

3 participants