Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Improve] Implement GetTopicAutoCreation #1151

Merged

Conversation

jiangpengcheng
Copy link
Contributor

@jiangpengcheng jiangpengcheng commented Jan 5, 2024

(If this PR fixes a github issue, please add Fixes #<xyz>.)

Fixes #

(or if this PR is one task of a github issue, please add Master Issue: #<xyz> to link to the master issue.)

Master Issue: #

Motivation

The GetTopicAutoCreation endpoint is missed, needs to add it

Modifications

Implement the GetTopicAutoCreation method

Verifying this change

  • Make sure that the change passes the CI checks.

  • Added some tests for topic auto-creation

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? (not applicable / docs / GoDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@@ -20,5 +20,5 @@ package utils
type TopicAutoCreationConfig struct {
Allow bool `json:"allowAutoTopicCreation"`
Type TopicType `json:"topicType"`
Partitions int `json:"defaultNumPartitions"`
Partitions *int `json:"defaultNumPartitions"`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not allowed to pass the partitions argument when the type is non-partitioned, so need to set this as a pointer

Copy link
Member

@RobertIndie RobertIndie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to add a test for it?

@jiangpengcheng
Copy link
Contributor Author

@RobertIndie There are no tests for the namespaces(and other modules) at all, we may need a PIP to setup the tests structure first

@RobertIndie
Copy link
Member

@RobertIndie There are no tests for the namespaces(and other modules) at all, we may need a PIP to setup the tests structure first

I don't think we need a PIP for that. We can utilize the existing testing framework. Seems that you can just create a test go file and write the test. I have created a PR to show how to do it: #1152

Then you can refer to these scripts to run the tests: https://github.com/apache/pulsar-client-go/tree/master/scripts . It will run in the CI: https://github.com/apache/pulsar-client-go/blob/master/.github/workflows/ci.yml#L50

@jiangpengcheng
Copy link
Contributor Author

jiangpengcheng commented Jan 5, 2024

I see, there will be a pulsar instance for testing, added some tests for topic auto-creation @RobertIndie

@RobertIndie RobertIndie merged commit 3388eae into apache:master Jan 9, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants