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

SOLR-17581: Less ZK watches in tests. #3036

Merged
merged 6 commits into from
Jan 20, 2025

Conversation

psalagnac
Copy link
Contributor

https://issues.apache.org/jira/browse/SOLR-17581

Description

Test creates lot of useless Zookeeper watches, because all waitForState() create a watch both on the collection state and the list of live nodes. Most of calls ignore live nodes when evaluating the predicate.

Solution

Introduce new test variant of waitForState(), that does not wait on live node changes when we're only interested in the collection state.

Tests

This is a test only change.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

Introduce new test variant of waitForState(), that does not wait on live node changes when we're only interested in
the collection state.
Copy link
Contributor

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

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

Nice!

return predicate.test(c);
});
} catch (Exception e) {
fail(message + "\n" + e.getMessage() + "\nLast available state: " + state.get());
Copy link
Contributor

Choose a reason for hiding this comment

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

didn't propagate 'e'; can't we throw it before logging the last available state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to do the same as existing variant of waitForState() (the one that watches both live nodes and the collection).
I think the two methods should be aligned. I think it makes sense to raise a JUnit specific error. Most of the time, the caught exception has no interesting details, it's just a timeout.

Copy link
Contributor

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

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

ZkStateReader.waitForState has the wait unit params immediately after the collection name. I think the new waitForState you added should be consistent. Keeping the functional arg at the end seems like better style as well.

@psalagnac
Copy link
Contributor Author

ZkStateReader.waitForState has the wait unit params immediately after the collection name. I think the new waitForState you added should be consistent. Keeping the functional arg at the end seems like better style as well.

Not new but fair! I dislike parameter order since I've been looking at these methods.

@psalagnac psalagnac merged commit bbbff85 into apache:main Jan 20, 2025
4 checks passed
@psalagnac psalagnac deleted the wait-for-state-variant branch January 20, 2025 19:19
psalagnac added a commit that referenced this pull request Jan 20, 2025
Introduce new test variant of waitForState(), that does not wait on live node changes when we're only interested in
the collection state. This is a test change only.

(cherry picked from commit bbbff85)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants