[Q] Wdyt of having some sort of getInternalAsyncId() or getSubrequestAsyncId() method in AdminCmdContext that would encapsulate this logic?

Seems like it's something a number of cmd implementations have to figure out for themselves, and would definitely fit under the general umbrella of "admin command context" conceptually.

So since this is mainly handled by the ShardRequestTracker, and very few Cmds actually create their own internal or sub request AsyncId directly, I don't think we really need a utility. Honestly I'm not sure these commands actually need to do it themselves at all. But not really in scope for this PR.

[+1] Good catch

This one I actually reverted, since it doesn't have any success or failure being returned (or an async option)

[-0] All of the "update" methods in this class shouldn't return SubResponseAccumulatingJerseyResponse, afaict, because the API response never uses the "successes" or "failures" fields that are the main reason for SubResponse... to exist.

If we aim of this change is to support async alias-prop modification, we can probably use AsyncJerseyResponse instead?

So weirdly only updateAliasProperties supports async currently, not even createOrUpdateAliasProperty. So for now only updateAliasProperties will use the AsyncJerseyResponse. The other two will use SolrJerseyResponse.

I will go through the other files and make sure I'm not changing any of these when I shouldn't.

[-0] Idk why L88 was instantiating a "SubResponseAccumulatingJerseyResponse" prior to this PR, maybe a copy-paste error on my part?

But in reality, this API never sets the "successes" or "failures" fields that are the main reason for SubResponse... to exist. So rather than switching the return type here to SubResponse, we should probably make it "SolrJerseyResponse" in both places? Or maybe "AsyncJerseyResponse" if we want to support asynchronous alias creation?

[0] Most (all?) of these mock-based v2 API tests have some version of this little snippet where we're grabbing the AdminCmdContext out of a capturer and asserting the action and async ID. Just flagging it as there might be a way to pull it into a helper method in MockAPITest.

Ok, I created a common validation method validateRunCommand that makes this really clean for all of the tests that do it. The method does everything after calling the api.

    validateRunCommand(
        CollectionParams.CollectionAction.DELETE,
        message -> {
          assertEquals(1, message.size());
          assertThat(message, hasEntry(NAME, "someCollName"));
        });

and

    validateRunCommand(
        CollectionParams.CollectionAction.DELETE,
        "someAsyncId",
        message -> {
          assertEquals(1, message.size());
          assertThat(message, hasEntry(NAME, "someCollName"));
        });

[0] Idk if "refresh" means anything to most users. Maybe isOperationFinished or something similar? Or maybe ignore me and "refresh" is the best option as anything else would risk causing confusion with the "COMPLETED" enum value.

Ignore me, just thinking aloud I guess...

I agree on "refresh" not being the most clear word. Maybe isFinal or isFinished, which implies you may want to get the state again.

Makes sense, I chose isFinal

I agree on "refresh" not being the most clear word. Maybe isFinal or isFinished, which implies you may want to get the state again.