[fix][broker] Gracefully shutdown does not work with admin cli in standalone

[JooHyukKim]

Fixes : #20617

Motivation

Currently, clients' shutdown API does not behave consistently in sense that asynchronicity is not handled explicitly. So issue #20617 happens.

This PR will allow clients know that shutdown is actually triggered.

Modifications

• Synchronize call to POST /shutdown on client side
• Asynchronize explicitly pulsar().closeAsync() invocation

Verifying this change

• Make sure that the change passes the CI checks.

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

Documentation

• doc
• doc-required
• doc-not-needed
• doc-complete

Matching PR in forked repository

PR in forked repository: https://github.com/JooHyukKim/pulsar/pull/17

Note

Here is link to the original PR, that ended up in messed up state wrt merge.

[Technoboy-]

A simple way to fix this is to wait the future.

getAdmin().brokers().shutDownBrokerGracefully(maxConcurrentUnloadPerSec, forcedTerminateTopic).join();

[Technoboy-]

doShutDownBrokerGracefullyAsync().thenAccept(__ -> asyncResponse.resume(Response.noContent().build()));

[JooHyukKim]

Ah, okay. I see where this is headed. Thank you! @Technoboy-

[Technoboy-]

And need to add exceptionally

[Technoboy-]

Also, we can use validateSuperUserAccessAsync

[JooHyukKim]

@Technoboy- thank you so much for the guidance. I should've thought of this solution earlier, before implementing this in more than 3 possible ways 🥲.

Done applying your review, though it seems clean and ready atm, I will see if there is anything else.

[coderzc]

It seems can't give a response after close, see:#14114 (comment)

[Technoboy-]

I find that we have many async calls in the cli, we'd better add a sync call the CmdBase:

protected long getReadTimeoutMs() {
        PulsarAdmin pulsarAdmin = getAdmin();
        if (pulsarAdmin instanceof PulsarAdminImpl) {
            return ((PulsarAdminImpl) pulsarAdmin).getClientConfigData().getReadTimeoutMs();
        }
        return 60000;
    }

    protected <T> T sync(Supplier<CompletableFuture<T>> executor) throws PulsarAdminException {
        try {
            return executor.get().get(getReadTimeoutMs(), TimeUnit.MILLISECONDS);
        } catch (InterruptedException e) {
            Thread.currentThread().interrupt();
            throw new PulsarAdminException(e);
        } catch (TimeoutException e) {
            throw new PulsarAdminException.TimeoutException(e);
        } catch (ExecutionException e) {
            throw PulsarAdminException.wrap(getApiException(e.getCause()));
        } catch (Exception e) {
            throw PulsarAdminException.wrap(getApiException(e));
        }
    }

And change this sync:

sync(() -> getAdmin().brokers().shutDownBrokerGracefully(maxConcurrentUnloadPerSec, forcedTerminateTopic));
            System.out.println("Successfully trigger broker shutdown gracefully");

What's your idea ? @codelipenghui @poorbarcode @mattisonchao @coderzc

[JooHyukKim]

I think Cmd classes are better off staying simple (not include sync/async) and users are also better off controlling only through CLI params.

EDITED And in this case maybe we can add parameter like --wait-for-completion, as suggested previously by @315157973 in #20651 (comment)

[labuladong]

doShutDownBrokerGracefullyAsync method will close the web service. After the web service is closed, can asyncResponse.resume(Response.noContent().build()); work? I think we can resume the response first, tell the admin CLI that the shutdown process has been triggered, finish the HTTP request. Then call doShutDownBrokerGracefullyAsync to do the shutdown. What do you think?

[JooHyukKim]

What do you think?

There is similar discussion above #20709 (comment) regarding synchronicity that we need to agree upon. Shall we discuss there also, and keep idea in one place?

[labuladong]

I don't think my question is similar to #20709 (comment). I mean, if we run doShutDownBrokerGracefullyAsync first, then run asyncResponse.resume(Response.noContent().build());, is that OK? In doShutDownBrokerGracefullyAsync method, the admin web service will be shut down, can asyncResponse.resume succeed when the web service has shut down?

I'm not sure if my guess is accurate, but I remember encountering a similar situation before. I suggest run asyncResponse.resume(Response.noContent().build()); first, then run doShutDownBrokerGracefullyAsync, to ensure the request can be finished in admin CLI.

[Technoboy-]

LGTM
@mattisonchao @codelipenghui @poorbarcode

[labuladong]

/pulsarbot rerun-failure-checks

[JooHyukKim]

/pulsarbot rerun-failure-checks

@labuladong Thank you for the rerun 👍🏻. The failure (Docker iamge build) has been fixed in #20831 , so the CI should work now.

[Anonymitaet]

@JooHyukKim this PR is labeled with doc-required, and explanations have been added to API code files. Does it mean that the doc has been already added?

[JooHyukKim]

Hmm, I don't think doc-required label is applicable anymore. The label must have been along the way.

WDYT, @Anonymitaet ?

[JooHyukKim]

Since there is no public API change, we might just move to doc-not-needed

[Anonymitaet]

OK