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

initial PR for simple admin client api and small admin client app #118

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

Conversation

yglcode
Copy link

@yglcode yglcode commented Apr 25, 2018

purpose:

  1. add simple admin api derived KIP117 and java code (https://github.com/confluentinc/kafka/tree/trunk/clients/src/main/java/org/apache/kafka/clients/admin)
  2. add client app, more consistent with Go cmdline (cobra) conventions, convenient for exploration of cluster resources, such as,
    kadm init -b kafka1:9092,kafka2:9092
    kadm topic list
    kadm topic create -p 10 -r 3 topic1 topic2 topic3
    kadm topic delete t1 t2 t3
    kadm node list
    of course, can still target command at specific broker:
    kadm group list --brokers kafka1:9092

Yigong Liu added 7 commits April 20, 2018 22:52
…from KIP 117 and Java code KafkaAdminClient at

"https://github.com/apache/kafka/tree/trunk/clients/src/main/java/org/apache/kafka/clients/admin". For a minimal footprint and close to protocol, reuse Jocko's fine Kafka protocol code and client interface code. Move client conn/dial code into a separate package. Other user code only need this client packge and protocol package (few hundreds Ks) to access AdminClient api. Sample of using this admin client api can be found at command line tool "kadmin", which support normal topics create/delete/list/describe,...
… as: kadm topic create -b kafka1:9092 -p 3 -r 2 topic1 topic2 topic3, kadm -b kafka1:9092,kafka2:9092 node list
@codecov
Copy link

codecov bot commented Apr 25, 2018

Codecov Report

Merging #118 into master will increase coverage by 0.35%.
The diff coverage is 9.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #118      +/-   ##
==========================================
+ Coverage    41.4%   41.76%   +0.35%     
==========================================
  Files          84       85       +1     
  Lines        5936     5572     -364     
==========================================
- Hits         2458     2327     -131     
+ Misses       3022     2801     -221     
+ Partials      456      444      -12
Impacted Files Coverage Δ
client/dialer.go 60% <ø> (ø)
client/state.go 0% <0%> (ø)
protocol/metadata_request.go 0% <0%> (ø) ⬆️
protocol/metadata_response.go 0% <0%> (ø) ⬆️
client/admin.go 0% <0%> (ø)
client/client.go 0% <0%> (ø)
jocko/broker.go 52.11% <100%> (-1.52%) ⬇️
jocko/leader.go 61% <100%> (+0.44%) ⬆️
client/conn.go 56.35% <61.53%> (ø)
... and 72 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3497f59...50f7416. Read the comment docs.

@travisjeffery
Copy link
Owner

Good work homie. Here's some layout changes to make:

  1. Let's move the conn and dialer back into the jocko pkg. We can export those to use/wrap from the client pkg. The jocko pkg shouldn't have to import the client pkg.
  2. Move the admin commands under the jocko cmd so there's one binary. I think:
jocko broker
jocko topic create|delete|list|describe
jocko group list|describe
jocko partition list|reset|describe
jocko node list|describe|api-version

Look good, similar setup Hashicorp uses.

When you've got those changes in place we can work on some style changes.

Thanks.

@yglcode
Copy link
Author

yglcode commented Apr 27, 2018

Thanks for your comments. Here are some more thoughts.

  1. Yes jocko/broker pkg shouldn’t need to import/pull-in client pkg which are most API handling. In fact jock/broker and client are quite independent, only sharing conn/dial code and protocol code. Protocol code has already been separated into its own pkg. We could move conn/dial code to a “conn” pkg, so that client and broker can build independently by importing “protocol” and “conn”, without pulling in other nonused code.

  2. I’ll add admin commands to jocko binary. It seems it is also good to keep a separate admin client binary, because jock broker code are more advanced and new features will take time to iron out. Client code is quite simple, mostly check KIPs and copy over Java code. Also can use admin client against apache Kafka servers and test if the protocol code is correct or not. If we keep them separate, then we can release them in diff schedule.

Thanks

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