Skip to content

[Test] Ensure assertion run only once #131174

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
Merged
merged 4 commits into from
Jul 15, 2025
Merged

[Test] Ensure assertion run only once #131174

merged 4 commits into from
Jul 15, 2025

Conversation

ywangd
Copy link
Member

@ywangd ywangd commented Jul 14, 2025

The assertion runs inside the cluster state listener may run a second time if the listener is not removed in time. It is possible that the assertion no longer holds on the 2nd time due to cluster state changes. There is no need to assert it more than once. This PR ensures assertion runs only once by removing the listener right after it.

Resolves: #130274

@ywangd
[Test] Ensure assertion runs only once
8863330 
The assertion runs inside the cluster state listener may run a second
time if the listener is not removed in time. It is possible that the
assertion no longer holds on the 2nd time due to cluster state changes.
There is no need to assert it more than once. This PR ensures assertion
runs only once by removing the listener right after it.

Resolves: elastic#130274
@ywangd ywangd requested review from pxsalehi and tlrx July 14, 2025 03:07
@ywangd ywangd added >test Issues or PRs that are addressing/adding tests :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) v9.1.0 v9.2.0 v8.19.1 v9.0.5 v8.17.10 v8.18.5 labels Jul 14, 2025
@ywangd ywangd changed the title [Test] Ensure assertion runs only once [Test] Ensure assertion run only once Jul 14, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Distributed Indexing Meta label for Distributed Indexing team label Jul 14, 2025
@ywangd ywangd mentioned this pull request Jul 14, 2025
Closed
ywangd
ywangd commented Jul 14, 2025
View reviewed changes
x-pack/plugin/ccr/src/internalClusterTest/java/org/elasticsearch/xpack/ccr/CcrRepositoryIT.java Outdated
.collect(Collectors.toList());
assertThat(sizes, hasSize(numberOfShards));
});
clusterService.removeListener(this);
Copy link
Member Author

@ywangd ywangd Jul 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Only this line is the real change. The others are due to using anonymous class (so that it can be referenced as this) instead of lambda.

pxsalehi
pxsalehi approved these changes Jul 14, 2025
View reviewed changes
Copy link
Member

@pxsalehi pxsalehi left a comment

Choose a reason for hiding this comment

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

You could probably use ClusterServiceUtils#addTemporaryStateListener?

@ywangd
Copy link
Member Author

ywangd commented Jul 15, 2025

I didn't use ClusterServiceUtils#addTemporaryStateListener initially because its current form takes a predicate and listeners accepting Void response. This means we need to get the cluster state again with ClusterService.state() in the asserting listener which felt a bit awkward to me.

That said, it technically works since all listeners run synchronously in the same thread so that the state returned by ClusterService.state() should the same as the one satisfied the predicate. Not using addTemporaryStateListener also begs the question on why not. So I pushed d6533e2 to use addTemporaryStateListener with a comment saying ClusterService.state() should return the same state. (The other option is to capture the state inside the predicate. But that also feels awkward in its own way)

@ywangd ywangd added the auto-backport Automatically create backport pull requests when merged label Jul 15, 2025
@ywangd
Copy link
Member Author

ywangd commented Jul 15, 2025

Just noticied that I accidently added auto-merge-without-approval before any review. I was meant to add auto-backport. Fortunately, the CI was flaky enough to prevent accidental merges. 😅

@elasticsearchmachine elasticsearchmachine merged commit 30d3877 into elastic:main Jul 15, 2025
33 checks passed
@ywangd ywangd deleted the es-130274-fix branch July 15, 2025 01:54
@ywangd ywangd mentioned this pull request Jul 15, 2025
Merged
ywangd added a commit to ywangd/elasticsearch that referenced this pull request Jul 15, 2025
@ywangd
[Test] Ensure assertion run only once (elastic#131174)
2c8d8c3 
The assertion runs inside the cluster state listener may run a second
time if the listener is not removed in time. It is possible that the
assertion no longer holds on the 2nd time due to cluster state changes.
There is no need to assert it more than once. This PR ensures assertion
runs only once by removing the listener right after it.

Resolves: elastic#130274
This was referenced Jul 15, 2025
Merged
Merged
ywangd added a commit to ywangd/elasticsearch that referenced this pull request Jul 15, 2025
@ywangd
[Test] Ensure assertion run only once (elastic#131174)
a5307b2 
The assertion runs inside the cluster state listener may run a second
time if the listener is not removed in time. It is possible that the
assertion no longer holds on the 2nd time due to cluster state changes.
There is no need to assert it more than once. This PR ensures assertion
runs only once by removing the listener right after it.

Resolves: elastic#130274
This was referenced Jul 15, 2025
Merged
Merged
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
✅ 9.1
✅ 8.19
✅ 9.0
✅ 8.17
✅ 8.18

ywangd added a commit to ywangd/elasticsearch that referenced this pull request Jul 15, 2025
@ywangd
[Test] Ensure assertion run only once (elastic#131174)
4cff008 
The assertion runs inside the cluster state listener may run a second
time if the listener is not removed in time. It is possible that the
assertion no longer holds on the 2nd time due to cluster state changes.
There is no need to assert it more than once. This PR ensures assertion
runs only once by removing the listener right after it.

Resolves: elastic#130274
ywangd added a commit to ywangd/elasticsearch that referenced this pull request Jul 15, 2025
@ywangd
[Test] Ensure assertion run only once (elastic#131174)
6cfa8a3 
The assertion runs inside the cluster state listener may run a second
time if the listener is not removed in time. It is possible that the
assertion no longer holds on the 2nd time due to cluster state changes.
There is no need to assert it more than once. This PR ensures assertion
runs only once by removing the listener right after it.

Resolves: elastic#130274
elasticsearchmachine pushed a commit that referenced this pull request Jul 15, 2025
@ywangd
[Test] Ensure assertion run only once (#131174) (#131260)
89fd454 
The assertion runs inside the cluster state listener may run a second
time if the listener is not removed in time. It is possible that the
assertion no longer holds on the 2nd time due to cluster state changes.
There is no need to assert it more than once. This PR ensures assertion
runs only once by removing the listener right after it.

Resolves: #130274
elasticsearchmachine pushed a commit that referenced this pull request Jul 15, 2025
@ywangd
[Test] Ensure assertion run only once (#131174) (#131259)
df00e82 
The assertion runs inside the cluster state listener may run a second
time if the listener is not removed in time. It is possible that the
assertion no longer holds on the 2nd time due to cluster state changes.
There is no need to assert it more than once. This PR ensures assertion
runs only once by removing the listener right after it.

Resolves: #130274
elasticsearchmachine pushed a commit that referenced this pull request Jul 15, 2025
@ywangd
[Test] Ensure assertion run only once (#131174) (#131256)
2c35e5c 
The assertion runs inside the cluster state listener may run a second
time if the listener is not removed in time. It is possible that the
assertion no longer holds on the 2nd time due to cluster state changes.
There is no need to assert it more than once. This PR ensures assertion
runs only once by removing the listener right after it.

Resolves: #130274
elasticsearchmachine pushed a commit that referenced this pull request Jul 15, 2025
@ywangd
[Test] Ensure assertion run only once (#131174) (#131257)
29d86ed 
The assertion runs inside the cluster state listener may run a second
time if the listener is not removed in time. It is possible that the
assertion no longer holds on the 2nd time due to cluster state changes.
There is no need to assert it more than once. This PR ensures assertion
runs only once by removing the listener right after it.

Resolves: #130274
elasticsearchmachine pushed a commit that referenced this pull request Jul 15, 2025
@ywangd
[Test] Ensure assertion run only once (#131174) (#131258)
46f9f5c 
The assertion runs inside the cluster state listener may run a second
time if the listener is not removed in time. It is possible that the
assertion no longer holds on the 2nd time due to cluster state changes.
There is no need to assert it more than once. This PR ensures assertion
runs only once by removing the listener right after it.

Resolves: #130274
mridula-s109 pushed a commit to mridula-s109/elasticsearch that referenced this pull request Jul 17, 2025
@ywangd @mridula-s109
[Test] Ensure assertion run only once (elastic#131174)
6fad7fe 
The assertion runs inside the cluster state listener may run a second
time if the listener is not removed in time. It is possible that the
assertion no longer holds on the 2nd time due to cluster state changes.
There is no need to assert it more than once. This PR ensures assertion
runs only once by removing the listener right after it.

Resolves: elastic#130274
mridula-s109 pushed a commit to mridula-s109/elasticsearch that referenced this pull request Jul 17, 2025
@ywangd @mridula-s109
[Test] Ensure assertion run only once (elastic#131174)
e397548 
The assertion runs inside the cluster state listener may run a second
time if the listener is not removed in time. It is possible that the
assertion no longer holds on the 2nd time due to cluster state changes.
There is no need to assert it more than once. This PR ensures assertion
runs only once by removing the listener right after it.

Resolves: elastic#130274
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pxsalehi pxsalehi pxsalehi approved these changes

@tlrx tlrx Awaiting requested review from tlrx

Assignees

No one assigned

Labels

auto-backport Automatically create backport pull requests when merged auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features Team:Distributed Indexing Meta label for Distributed Indexing team >test Issues or PRs that are addressing/adding tests v8.17.10 v8.18.5 v8.19.1 v9.0.5 v9.1.0 v9.2.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[CI] CcrRepositoryIT testCcrRepositoryFailsToFetchSnapshotShardSizes failing

3 participants

@ywangd @elasticsearchmachine @pxsalehi