SOLR-17971: Fix REBALANCELEADERS wait when the Overseer is disabled

[sarkaramrit1993]

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

With the Overseer disabled, REBALANCELEADERS always waited the full maxWaitSeconds (60s by default) and then logged a timeout. The leaders still moved. waitAsyncRequests() only looked for sub-command results in the Overseer completed/failure maps, but in distributed mode those results go to DistributedApiAsyncTracker. Because of that, the ids were also never cleared and piled up under /distributedapi/async/persistent.

Changes:

• waitAsyncRequests() now checks the store for the current mode, same branching as REQUESTSTATUS in ClusterCommands. In distributed mode, deleteSingleAsyncId() both tells us the task is done and removes it.
• Overseer path behaves the same, just drops the extra contains() before remove().

Not changed: I went with the tracker directly instead of issuing a REQUESTSTATUS call per id as the JIRA suggests. It's the same lookup without the extra request.

Tests: new testRebalanceLeadersClearsAsyncIds in TestRebalanceLeaders asserts no rebalanceleaders_ ids are left in either store and the call returns before the timeout. Without the fix and the Overseer disabled, it fails with 29 leftover ids. Ran the full class with -Ptests.nightly=true in both modes, plus gradlew check -x test.

[dsmiley]

Looks good!

The fix branches on the mode the same way REQUESTSTATUS does in ClusterCommands

Then maybe the logic should be centralized? (didn't check how elegant this is)

[sarkaramrit1993]

Good call, done. Both REBALANCELEADERS and DELETESTATUS now go through ZkController.removeStoredAsyncResponse(asyncId). The REQUESTSTATUS lookup and delete-all paths still do their own branching. Nothing else duplicates them, so I left those alone.

[psalagnac]

This test does exactly the same assersions when running with the overseer enabled of disabled. Since it checks these internals, it may be worth to also have different assertions in the test.

[psalagnac]

I'm not a fan of having magic constants like that in a test. It does assumptions on how we store the distributed map in ZK, and the test might be obsolete in case of some changes, without anyone one noticing it.

[sarkaramrit1993]

Thanks @psalagnac, good catches. Reworked the test in the latest commit:

• Goes through ZkController and Overseer.getAsyncIdsMap() now, no ZK paths or prefixes.
• Overseer enabled: snapshots the completed, failure and claimed async ids before the rebalance, and checks nothing new is left after.
• Overseer disabled: checks the command returns well before maxWaitSeconds, since the wait only ends early once every sub-command's stored response is removed.
• Also asserts at least one leader change was requested, so it can't pass on a no-op.

Distributed mode has no direct leak check, on purpose. There's no public way to list what the async tracker holds and the sub-command ids never reach the client, so a direct check means either ZK internals again or test-only production code. The removal goes through the same ZkController.removeStoredAsyncResponse as DELETESTATUS, which DeleteStatusTest already covers in that mode.

Verified it fails against the original code in distributed mode, and against versions that skip removing or releasing the ids in Overseer mode.

[psalagnac]

Thanks for the cleanup.
It's unfortunate that we don't check whether async task IDs are cleaned up as expected when the overseer is disabled.

Any idea what it would take to test this? Since they're supposed to remain internal, it might not be worth making too much things public just to verify it.

[sarkaramrit1993]

Took a look, it needed two small read-only accessors. Sub-command ids are rebalanceleaders_<core>_<nanoTime>, so the test can't guess them and ask REQUESTSTATUS. It needs a way to list what the tracker holds.

Pushed in 52b2834:

• DistributedApiAsyncTracker.getTrackedAsyncIds() returns trackedAsyncTasks.keys()
• DistributedCollectionConfigSetCommandRunner.getTrackedAsyncIds() passes it through, reached via ZkController.getDistributedCommandRunner()

Both @VisibleForTesting, no behavior change. The test now snapshots the async ids before the call and diffs after in both modes, so the disabled-Overseer branch gets a direct leak check too.

Checked it fails when the wait ends without deleting the ids (lists the 6 leftover rebalanceleaders_* ids), and passes in both modes.

[psalagnac]

Thanks for the patch. I'll merge this in a couple of days.