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

[ISSUE #3369][FEAT]create topic with broker addr or name or none #1087

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Tomilla
Copy link

@Tomilla Tomilla commented Aug 19, 2023

What is the purpose of the change

Create Topic with optional argument:

  • broker address(current implement is required argument): direct invoke createTopic API with address
  • broker name: in first, client will call getBrokerClusterInfo, then resolve name to address, then invoke API
  • nothing: like broker name case, but it will apply createTopic on all addresses that getBrokerClusterInfo returned.

Brief changelog

  1. implement GetBrokerClusterInfo of Admin interface. in the meanwhile, there are integer type keys in RocketMQ Java Server response of GetBrokerClusterInfo, Golang JSON library cannot unmarshal them directly. So I fixed the JSON didn't conform to the JSON specification (JSON keys should always be strings, don't use int as a key).

//GetBrokerClusterInfo(ctx context.Context) (*remote.RemotingCommand, error)

  1. introduce another CreateTopic implementation like sarama, without brokerAddr as input.

// TODO: another implementation like sarama, without brokerAddr as input

  1. add optional flag for createTopic to avoid override same name topic configuration. for back compatibility, it is false by default.

Verifying this change

I've update the examples/admin/topic/main.go file, which demonstrates all new features such as:

  • get broker cluster info;
  • create topic without broker address;
  • create topic with "NotOverride" check.

furthermore, I also wrote UnitTest and BenchmarkTest for JSON integer key rectify function.

Related to apache/rocketmq#3369

Follow this checklist to help us incorporate your contribution quickly and easily. Notice, it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR.

  • Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit-test(over 80% coverage) to verify your logic correction, more mock a little better when a cross-module dependency exists.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

@Tomilla
Copy link
Author

Tomilla commented Aug 19, 2023

Compare this to @Slideee PR (#1006), which tried to import a go-version JavaScript interpreter into the project, but that was only used in very small cases (json loading), so it is overkill for our limited needs.

My solution is green and doesn't introduce any 3rd party heavyweight modules. furthermore, since it all uses native syntax or built-in libraries, it is better performance than adding a VM layer.

Related to #1005

@HubQin
Copy link
Contributor

HubQin commented Mar 12, 2024

it's a nice feature!

@Tomilla
Copy link
Author

Tomilla commented Mar 13, 2024

@HubQin
it's a nice feature!

The code for this PR has been running stably within our company for more than half a year, indicating that there is no problem with the logic of the code itself.

However, It is unclear what the maintainer @xujianhai666 thinks about this feature merge request? If there is a need for more test cases, or additional documentation or something, please let me know exactly what to do.

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.

2 participants