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-17300: Copy existing listeners on re-creation of Http2SolrClient #2467

Merged

Conversation

iamsanjay
Copy link
Contributor

@iamsanjay iamsanjay commented May 18, 2024

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

Instead of #2462, this PR would copy the existing listeners to the new Http2SolrClient. This way re-creating client not requires to call explicitly Builder#withListenerFactory to copy existing listeners as withHttpClient method automatically takes care of that.

Added #2462 test case here.

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)
  • 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

@iamsanjay iamsanjay changed the title SOLR-16505: Copy existing listeners on re-creation of Http2SolrClient SOLR-17300: Copy existing listeners on re-creation of Http2SolrClient May 20, 2024
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.

Proposed CHANGES.txt under improvements:

SOLR-17300: Http2SolrClient.Builder.withHttpClient now copies HttpListenerFactory (e.g. for auth, metrics, traces, etc.)

@BeforeClass
public static void setupCluster() throws Exception {
cluster =
configureCluster(2)
Copy link
Contributor

Choose a reason for hiding this comment

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

will the test do its job with one node? If so, reduce resources to help make our tests faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes one node will do. +1 to reduce resources!

Copy link
Contributor

Choose a reason for hiding this comment

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

Someday in dev-docs, we ought to have "The list of things @dsmiley looks for when writing tests" ;-)

@dsmiley
Copy link
Contributor

dsmiley commented May 20, 2024

Hmmm TestInterleavingTransformer failed and it isn't a flapper:

@iamsanjay
Copy link
Contributor Author

Hmmm TestInterleavingTransformer failed and it isn't a flapper:

Yup it's new. But ran successfully on my machine, tried multiple times now. Should we consider enabling the re-running failing test strategy, or I do not know If it's enabled already or not, to check whether it's flaky or not? https://docs.gradle.com/develocity/flaky-test-detection/

Evidence To further support it:

Flaky Tests at Google and How We Mitigate Them

We even have a way to denote a test as flaky - causing it to report a failure only if it fails 3 times in a row. This reduces false positives, but encourages developers to ignore flakiness in their own tests unless their tests start failing 3 times in a row, which is hardly a perfect solution

Cost of Flaky Tests in CI: An Industrial Case Study

Automatically rerunning a test costs 0.02~cents, while not rerunning and thus letting the pipeline fail results in a manual investigation costing $5.67 in our context. The insights gained from our case study have led to the decision to shift effort from investigation and repair to automatically rerunning tests

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.

Flaky test detection & running a failing test again is a topic that should be brought to the dev list. I'd absolutely love to see that be worked on! Would probably start with us using https://github.com/gradle/test-retry-gradle-plugin which is referred to by GE.

You should trigger a re-run of the failed build; it doesn't take long for Crave's 96-core beast to run all tests.

Any way, +1 for you to merge with your newfound privileges to do so. Take care to edit the commit message to clean it up. I like to take the CHANGES.txt text and paste it in and maybe edit it a bit, and then add further detail if warranted (e.g. why you deprecated something).

If you don't have privileges to do these things then you have yet to complete your onboarding instructions involving GitHub.

@iamsanjay iamsanjay self-assigned this May 21, 2024
@iamsanjay iamsanjay merged commit 3837eeb into apache:main May 21, 2024
4 checks passed
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.

3 participants