janhoy commented on PR #3938:
URL: https://github.com/apache/solr/pull/3938#issuecomment-3638372711
> There isn't anything to help protect the poor human being writing a test
from using the `cluster.getSolrClient()` method...
I think there is nothing wrong in general in using the cluster client. But
the flaky pattern being used in this particular test is imo the reliance of
`LogListener`, i.e. counting number of admin commands in the logs, see comment
on top of original test method:
```java
// This ensures CH2SC is caching cluster status by counting the number
of logged calls to the
// admin endpoint. Too many calls to CLUSTERSTATUS might mean
insufficient caching and
// performance regressions!
// BaseHttpClusterStateProvider has a background job that pre-fetches
data from CLUSTERSTATUS
// on timed intervals. This can pollute this test, so we set the
interval very high to
// prevent it from running.
```
The comment says explicitly that it is brittle. I don't know for sure
whether Claude made it less brittle, but it sounds plausible that the
`SYS_PROP_CACHE_TIMEOUT_SECONDS` property must be set to `Integer.MAX_VALUE`
**before** we instantiate the client. But if the cluster level client is
**already** instantiated before the test method starts, it won't work and you
get more pollution, and then the exact check `assertEquals(2,
adminLogs.getCount());` cannot be trusted, and you get flakiness.
I'll let Copilot judge Claude :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
