[PR] [fix][test] Fix order-dependent AdminApiTest.testUpdateDynamicLoadBalancerSheddingIntervalMinutes [pulsar]

["merlimat (via GitHub)" <gi...@apache.org>]

merlimat opened a new pull request, #26872:
URL: https://github.com/apache/pulsar/pull/26872

   ### Motivation
   
   `AdminApiTest.testUpdateDynamicLoadBalancerSheddingIntervalMinutes` fails whenever another test method of the class runs before it on the same broker, which is the case in every full-class run. It passes when run alone, and CI's default `testRetryCount=1` hides the failure: `TestRetrySupport` re-runs the class setup before the retry, and the retry passes.
   
   The test updates the `loadBalancerSheddingIntervalMinutes` dynamic configuration and waits for the test class's `conf` field to show the new value. But `conf` is the broker's configuration only until the first test method ends. The `@AfterMethod` `resetClusters()` calls `resetConfig()`, which assigns a new `ServiceConfiguration` to `conf`, while the running broker keeps the object it was started with. The broker applies dynamic configuration updates to its own object (`BrokerService#configValueChanged` sets the field on `pulsar.getConfiguration()`), so `conf` never changes and the wait times out. This has been the case since #17980 moved the class from a broker per test method to a broker per class.
   
   On master, running any test method before it reproduces the failure:
   
   ```
   ./gradlew :pulsar-broker:test -PtestRetryCount=0 \
       --tests "org.apache.pulsar.broker.admin.AdminApiTest.brokers" \
       --tests "org.apache.pulsar.broker.admin.AdminApiTest.testUpdateDynamicLoadBalancerSheddingIntervalMinutes"
   ```
   
   ```
   org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in org.apache.pulsar.broker.admin.AdminApiTest was not fulfilled within 10 seconds.
   	at org.apache.pulsar.broker.admin.AdminApiTest.testUpdateDynamicLoadBalancerSheddingIntervalMinutes(AdminApiTest.java:593)
   ```
   
   The broker logs `Successfully updated configuration {configKey=loadBalancerSheddingIntervalMinutes, newValueStr=10}` right after the update. The code path:
   
   ```java
   @AfterMethod(alwaysRun = true)
   public void resetClusters() throws Exception {
       ...
       resetConfig();        // MockedPulsarServiceBaseTest: this.conf = getDefaultConf();
       applyDefaultConfig();
       setupClusters();
   }
   
   public void testUpdateDynamicLoadBalancerSheddingIntervalMinutes() throws Exception {
       admin.brokers().updateDynamicConfiguration("loadBalancerSheddingIntervalMinutes", "10");
       Awaitility.await().until(() -> {
           return conf.getLoadBalancerSheddingIntervalMinutes() == 10;   // not the broker's configuration
       });
       assertEquals(conf.getLoadBalancerSheddingIntervalMinutes(), 10);
   }
   ```
   
   ### Modifications
   
   Check the configuration the broker runs with, `pulsar.getConfiguration()`, as the other dynamic-configuration tests of the class already do:
   
   ```java
   Awaitility.await().until(() -> {
       return pulsar.getConfiguration().getLoadBalancerSheddingIntervalMinutes() == 10;
   });
   
   assertEquals(pulsar.getConfiguration().getLoadBalancerSheddingIntervalMinutes(), 10);
   ```
   
   The test still fails if the broker doesn't apply the update, since the default value is `1`. No production code is changed.
   
   ### Verifying this change
   
   - [ ] Make sure that the change passes the CI checks.
   
   This change is already covered by existing tests, such as `AdminApiTest`. With `-PtestRetryCount=0`:
   
   - The two-method run above fails on master and passes with this change.
   - The full `AdminApiTest` class passed in 3 of 3 runs with this change (99 tests, 0 failures, and no `TestRetrySupport` re-setup during the run).
   
   ### Does this pull request potentially affect one of the following parts:
   
   *If the box was checked, please highlight the changes*
   
   - [ ] Dependencies (add or upgrade a dependency)
   - [ ] The public API
   - [ ] The schema
   - [ ] The default values of configurations
   - [ ] The threading model
   - [ ] The binary protocol
   - [ ] The REST endpoints
   - [ ] The admin CLI options
   - [ ] The metrics
   - [ ] Anything that affects deployment
   


-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [fix][test] Fix order-dependent AdminApiTest.testUpdateDynamicLoadBalancerSheddingIntervalMinutes [pulsar]

["merlimat (via GitHub)" <gi...@apache.org>]

merlimat merged PR #26872:
URL: https://github.com/apache/pulsar/pull/26872


-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org