You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "flowchartsman (via GitHub)" <gi...@apache.org> on 2023/08/30 23:33:37 UTC
[GitHub] [pulsar-client-go] flowchartsman opened a new pull request, #1085: [Issue 1075][pulsaradmin] Reorganize
flowchartsman opened a new pull request, #1085:
URL: https://github.com/apache/pulsar-client-go/pull/1085
Master Issue: #1075
### Motivation
This PR represents the changes first proposed by me on #streamnative/pulsar-admin-go#40 and #streamnative/pulsar-admin-go#39 squashed and replayed on top of master after #1079 landed.
### Modifications
The API has been thoroughly rehashed, with most subpackages removed in favor of a cleaner top-level API on the base package which is much friendlier to autocomplete.
In addition, every REST endpoint can now be explicitly versioned.
It also includes the following changes:
https://github.com/flowchartsman/pulsar-admin-go/commit/ef9273fec5604c905801722b563c9f9b5ee922d7
https://github.com/flowchartsman/pulsar-admin-go/commit/c74ba31f9dca72002349c8322d286747e8984209
https://github.com/flowchartsman/pulsar-admin-go/commit/97989ac7845a6032c34910a5de2b2dfa8bd269bc
https://github.com/flowchartsman/pulsar-admin-go/commit/feace756d98638e8acf797ae808c533a027fa76d
https://github.com/flowchartsman/pulsar-admin-go/commit/756acd897ed455095b08651747321eadf69acd35
https://github.com/flowchartsman/pulsar-admin-go/commit/2bd7185b8a3f1e7f614b8c8911155829abbe16b6
### Verifying this change
This PR includes some additional tests, however `pulsaradmin` as a whole is far from fully exercised, however I have a plan for testing (see next section).
### Proposed Changes To Integration and Testing
Once this PR is complete (or as part of it), I plan to convert all of the raw API calls in the main library test code to use an integration fixture `pulsaradmin` client instead, which will help exercise the code in real-world use.
In addition, I would like to combine this with the changes I started in #1076 to remove the need for makefile-driven testing in favor of using a Docker-based IT framework such as https://github.com/testcontainers/testcontainers-go. There would be several benefits to this approach, chief among them that tests could be run piecemeal, and only those tests which actually use the integration container would cause it to become initialized. This would improve feature iteration across the board.
### Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): shockingly, _no_.
- The public API: **yes**
- As mentioned above, this is a rather significant change to the `pulsaradmin` API
- The schema: no
- The default values of configurations: no
- The wire protocol: no
### Documentation
While this PR does not introduce a new feature per se, any existing examples using the old `pulsaradmin` API would need to be modified.
### Remaining work
There is one remaining point I need to address before this PR is complete, and several nice-to-haves.
- **REQUIRED** Config/Auth merging
- The auth and config for `pulsaradmin` are still completely separate, and use a different initialization strategy to the main package. They will need to DRYed with the code in `pulsar/auth
- **NICE-TO-HAVE** Make parameters more consistent
- There are still some rough edges and inconsistencies in the API, particularly in regards to method paramaters. One example is methods that consume types like [`TopicName`](https://github.com/apache/pulsar-client-go/blob/master/pulsaradmin/pkg/utils/topic_name.go#L35-L43) and [`NamespaceName`](https://github.com/apache/pulsar-client-go/blob/master/pulsaradmin/pkg/utils/namespace_name.go#L28-L31). I understand the desire for these types, but they are inconsistently applied, since elsewhere the API allows you to pass in strings for either of these types, sometimes separated, sometimes combined. This should be consistent.
- Furthermore the control over contents that the getter functions might appear to provide is not as precise as it would be in Java, thanks to value literals.
- It is also awkward to call their [`Get-` functions](https://github.com/apache/pulsar-client-go/blob/master/pulsaradmin/pkg/utils/topic_name.go#L47-L99) only to immediately need to check or discard an error and then to dereference, since the methods [consume values](https://github.com/apache/pulsar-client-go/blob/master/pulsaradmin/pkg/admin/topic.go#L44), while the `Get-` functions [return pointers](https://github.com/apache/pulsar-client-go/blob/master/pulsaradmin/pkg/utils/namespace_name.go#L33) This is cumbersome, and can be simplified by externalizing the fields and allowing literal init, or by changing the getter functions to return value types instead (along with better names), and simply having the methods validate the arguments themselves. This reduces two error checks and a dereference down to one error check on the method call itself. In theory, these types could even be converted to string types with some extra methods, without too much trouble.
- **NICE-TO-HAVE** This API represents a great opportunity for integration into the testing code for package `pulsar` itself, since most of the raw API calls made in that code are administrative. Combining this with testcontainers-go and an integration harness would absolutely _slap_.
- **NICE-TO-HAVE** I note that the transports in `pulsar/auth` [do not clone the requests](https://github.com/apache/pulsar-client-go/blob/master/pulsar/auth/athenz.go#L252-L259C2), which is a [modification I have made](https://github.com/flowchartsman/pulsar-admin-go/blob/master/internal/auth/oauth2.go#L162C50-L162C50) to the auth handlers here, and which is [highly encouraged](https://github.com/golang/go/blob/master/src/net/http/client.go#L129-L133) by the standard library to allow requests to be retried, among other uses.
--
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
[GitHub] [pulsar-client-go] flowchartsman commented on pull request #1085: [Issue 1075][pulsaradmin] Reorganize pulsaradmin codebase
Posted by "flowchartsman (via GitHub)" <gi...@apache.org>.
flowchartsman commented on PR #1085:
URL: https://github.com/apache/pulsar-client-go/pull/1085#issuecomment-1708888007
If you're curious about the API and migration, I think you would find it rather easy, here are some snippets to give you an idea:
```go
padminClient, err := pulsaradmin.NewClient(flagPulsarWebServiceURL, flagPulsarClusterURL, flagPulsarAuthToken)
if err != nil {
return nil, fmt.Errorf("error creating pulsar admin client: %w", err)
}
// listing topics
ns, _ := pulsaradmin.GetNameSpaceName(myTenant, myNamespace) // like I mentioned above, this is something I also want to fix, which would be in those later PRs you talked about
topicsPartitioned, topicsUnpartitioned, err := padminClient.Topics().List(*ns)
if err != nil {
return fmt.Errorf("unable to pull list of topics for namespace %s: %v", myNamespace, err)
}
// checking function status
functionStatus, err := s.pulsarAdminClient.Functions().GetFunctionStatus(common.TenantFlows, common.NamespaceAgents, uuid)
if pulsaradmin.IsNotFound(err) { // this is also new
// handle not found separately rather than returning an error
}
if err != nil {
return fmt.Errorf("unexpected error retrieving function status: %v", err)
}
// creating a function
funcURL := "function://myTenant/myNamespace/name_of_fn@1.0"
functionConfig := &pulsaradmin.FunctionConfig{
CleanupSubscription: true,
AutoAck: true,
LogTopic: logtopic,
ProcessingGuarantees: "ATLEAST_ONCE",
Tenant: myTenant,
Namespace: myNamespace,
Name: functionName,
Inputs: []string{inputTopic},
Output: outputTopic,
ClassName: "name_of_fn", //not necessary, but prettier
ForwardSourceMessageProperty: true,
Go: &funcURL,
SubscriptionPosition: "Latest",
}
if err := s.padminClient.Functions().CreateFuncWithURL(functionConfig, funcURL); err != nil {
return false, fmt.Errorf("couldn't create the function: %w", err)
}
```
--
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
[GitHub] [pulsar-client-go] tisonkun commented on pull request #1085: [Issue 1075][pulsaradmin] Reorganize pulsaradmin codebase
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #1085:
URL: https://github.com/apache/pulsar-client-go/pull/1085#issuecomment-1712781273
> could you elaborate on that further
I mean that when you include some nontrivial changes like:
* Add API versions field;
* Refactor Error structs;
* ...
with a > 3000 lines refactor where most of its changes are rename, it's very hard for downstream developers who find the interface changed to figure out the exact difference before and after this patch - they should find the significant 20-40 line in these 3000+ lines, with no dedicated commit hint.
Anyway, I'm OK to merge this patch and go once more turn to migrate the downstream projects.
You refer to an outstanding issue about unify "auth". Do you want to include more nontrivial changes in this patch? I'll strongly suggest you submit a separate patch since we don't release per PR but we have time to finish your full proposal in several PRs (you can submit them at once and we do a simultaneous review, see https://github.com/apache/flink/pull/9832 as an example, multiple commits or PRs instead of one commit for everything). Once a PR goes over thousands lines and contain a few lines nontrivial, I'm afraid that there is no more reviewers but we "merge and bless".
--
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
[GitHub] [pulsar-client-go] flowchartsman commented on pull request #1085: [Issue 1075][pulsaradmin] Reorganize pulsaradmin codebase
Posted by "flowchartsman (via GitHub)" <gi...@apache.org>.
flowchartsman commented on PR #1085:
URL: https://github.com/apache/pulsar-client-go/pull/1085#issuecomment-1708855952
I list the changes above, however, the main changes to the API are client creation and auth, which I intended on replacing with the stuff from `pulsar/auth` once I had some feedback. Once that is incorporated there won't be a lot of major changes.
The only other big change is the addition of the versioning for every endpoint, which allows pulsar functions to work again, since they were broken with `pulsar-admin-go` in recent versions due to it hitting the v2 endpoint, however this is internal and it has a default value set, so it is not required to create the client.
I am unsure what you mean by
| I'd suggest we fix the package layout as a flattened one + internal package
Since this is already a flat layout could you elaborate on that further? If the idea is for me to back out any changes that weren't moving packages around, I would rather avoid that if I can and just fix auth here. It was already a pain to recreate the changes I did to `pulsar-admin-go` and it took me the better part of a day to replay them all in a way that made sense, and I had to go through and manually `checkout -p` all of the changes to the license comments so that it applied cleanly.
If I absolutely must do that, I will, but I want to be very clear on what's being asked of me before I go through all that effort a third time.
--
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
[GitHub] [pulsar-client-go] nodece commented on pull request #1085: [Issue 1075][pulsaradmin] Reorganize pulsaradmin codebase
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #1085:
URL: https://github.com/apache/pulsar-client-go/pull/1085#issuecomment-1713119800
Thank you for your contribution! Your code looks good to me, but this PR is large, and includes changes to the feature and file movement.
Could you split multiple PRs?
--
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