Tracking issue for merging pulsar-admin-go

tisonkun commented 1 year ago

Background

Vote thread - https://lists.apache.org/thread/txpxdkzf4n5dd6bm0goqxfycr69g9gc4
IP clearance - https://lists.apache.org/thread/85hynjryvr2869v9wtrqb8ym1853vx88

Subtasks

[x] Roughly add source code with ASF header
[x] Port workflows and other settings
[ ] Refactor auth logics

tisonkun commented 1 year ago

@flowchartsman I outline the subtasks above and will start roughly adding source code the next week. You may propose the refactor ideas and I can help in review.

cc @RobertIndie @shibd @nodece @wolfstudy

flowchartsman commented 1 year ago

Thanks @tisonkun! We might want to open the issue for discussion first before we issue PRs, since the current API and mine are fairly different, and it might help to avoid duplicated effort, but I'm game to do it however you like.I can convert my draft PR to a regular one first?

For those interested, I have a refactoring issue and accompanying PR in place for pulsar-admin-go that removes most of the package divisions, unifying the types and API calls into a single root package as well as refactoring http client and auth code to into internal, to avoid cluttering the namespace and exposing auth via a functional API. This avoids the necessity of importing multiple pkg packages to perform any one action, and avoids exposing any internals to the user that they will have no use for. Many of these changes will apply to a merger in pulsar-client-go.

The PR also adds package-level error interfaces along with helper methods to distinguish missing/not found entities from actual errors, as well as "expected" errors which conform to the admin API (i.e. with a reason) versus those errors that represent more exceptional circumstances. The internal error types are not exposed to the user; instead the interfaces can be checked against an error value using the standard library errors.As.

Notes on merging

I would imagine most of the API would stay the same and could be called through admin methods on the client type, though it might be reasonable to add an exported Admin field (as an interface in keeping with the mores of this package) to keep the method count down and separate the functionality.
Because pulsar-client-go already handles (most) auth plugins natively, the functional auth API I designed for my PR would no longer be necessary, and authentication would come from the auth settings specified in pulsar.ClientConfig
There is relatively minor testing in pulsar-admin-go, and no integration testing yet. My plan was for my PR is to exercise most if not all endpoints using an actual pulsar container and testcontainers-go. This is a flexible integration testing strategy that allows tests to be run individually using the -run flag to go test, automatically spinning up the integration container only when it is first exercised by a test that requires it, and automatically stopping and removing the test container with a reaper container when testing has stopped for any reason. This is opposed to creating the container and testing every test, every time, as is currently the approach for pulsar-client-go. I have had great success with this method internally, and, in fact, I have a draft PR for pulsar-client-go that sets the groundwork for this testing strategy.

Once integrated, one way to call the adminAPI might be directly from the Client type, like so:

        client, err := pulsar.NewClient(ClientOptions{
        URL:                   serviceURLTLS,
        TLSTrustCertsFilePath: caCertsPath,
                TLSValidateHostname:   true,
        Authentication: NewAuthenticationToken(string(token)),
    })
        err := client.Admin.Functions().CreateFuncWithURL(funcPkgURL, &pulsar.FunctionConfig{
        Output:     funcResults,
        LogTopic:   funcLog,
        Tenant:     funcTenant,
        Namespace:  funcNamespace,
        Name:       funcName,
        Inputs:     []string{funcInput},
        Runtime:    "GO",
        }
        if err != nil {
                // handle error
        }

Note the use of Functions() as a function which returns ~a particular API execution type~ a runner for the /v3/functions API. This could just as easily be replaced with a field on the Admin type.

tisonkun commented 1 year ago

Thanks for your feedback and participation @flowchartsman!

I've submitted a PR https://github.com/apache/pulsar-client-go/pull/1077 for roughly adding the sources of pulsar-admin-go, so that we have a baseline to improve and the downstream projects can first migrate to that hash for updating module/pkg path, and later catch up any refactors.

Your refactoring suggestions can be applied onto that patch.

package-level error interfaces along with helper methods

I like this.

exported Admin field

As we discussed on Slack, perhaps a parallel interface design for client API and admin API can be better. This is the design of Java APIs and it reflects that admin APIs (control panel) are at the same level of client API (data panel) - client APIs don't "own" admin APIs. Also, this can make the auth model clear - generally, users configure less permission for client APIs while admin APIs requires high trust level.

Because pulsar-client-go already handles (most) auth plugins natively, the functional auth API I designed for my PR would no longer be necessary, and authentication would come from the auth settings specified in pulsar.ClientConfig

This is the right direction. We will factor out shared/common configs and the auth package. But how to reduce the impact for current users is a point that needs a second consideration.

exercise most if not all endpoints using an actual pulsar container

This is cool! We definitely need it.

apache / pulsar-client-go