adevinta / zoe

The Kafka CLI for humans
https://adevinta.github.io/zoe
MIT License
286 stars 21 forks source link

Include topic config properties in describe topic command #4

Closed mgmeiner closed 4 years ago

mgmeiner commented 4 years ago

When describing a topic in zoe it would be useful to also display the config properties of it.

This PR introduces this feature by extending the TopicDescription class with the config properties as map.

When describing a topic the current implementation in zoe just loads the descriptions of all topics and then just returns the description of the requested topic. This remains the same with this PR. I tested it on a local kafka cluster with 11 topics and there was no significant change in performance, so i think its ok :).

I also adjusted the cli format method when requesting output format json. It now pretty-prints the json as i guess this is the desired result.

Example result of a topic description now when calling:

zoe -c local -o json topics describe someTopic

Result:

  "topic" : "someTopic",
  "internal" : false,
  "partitions" : [ 0, 1 ],
  "config" : {
    "cleanup.policy" : "compact",
    "compression.type" : "producer",
    "delete.retention.ms" : "100",
    "file.delete.delay.ms" : "60000",
    "flush.messages" : "9223372036854775807",
    "flush.ms" : "9223372036854775807",
    "index.interval.bytes" : "4096",
    "max.message.bytes" : "1000012",
    "message.downconversion.enable" : "true",
    "message.format.version" : "2.1-IV2",
    "message.timestamp.difference.max.ms" : "9223372036854775807",
    "message.timestamp.type" : "CreateTime",
    "min.cleanable.dirty.ratio" : "0.01",
    "min.compaction.lag.ms" : "100",
    "min.insync.replicas" : "1",
    "preallocate" : "false",
    "retention.bytes" : "-1",
    "retention.ms" : "604800000",
    "segment.bytes" : "1073741824",
    "segment.index.bytes" : "10485760",
    "segment.jitter.ms" : "0",
    "segment.ms" : "604800000",
    "unclean.leader.election.enable" : "false"
  }
}

If something is missing or needs to be adjusted, just let me know. i will update the PR.

Also thanks for the work on zoe 👌 !
Working now for a week with it and have not looked back to the original kafka-cli since then (except to see the configs of a specific topic)

wlezzar commented 4 years ago

Thanks a lot for this contribution @mgmeiner and your feedback : ) . This is indeed a very useful addition to the topics describe command. I will take a look at your PR today and then we should be good for merge :+1:

mgmeiner commented 4 years ago

Alright ☺️!

If you like I will update this PR to show only dynamic-config-props which have been explicitly set during topic creation like 'cleanup.policy', 'delete.retention.ms' and so on. This would be the same behavior as implemented in kafka-cli.

I would add a new cli option '--all' for the topic describe command which would then display all config props including the default ones set on the broker.

Let me know what you think.

wlezzar commented 4 years ago

If you like I will update this PR to show only dynamic-config-props which have been explicitly set during topic creation like 'cleanup.policy', 'delete.retention.ms' and so on. This would be the same behavior as implemented in kafka-cli.

I would add a new cli option '--all' for the topic describe command which would then display all config props including the default ones set on the broker.

Let me know what you think.

I think it's better to have the full list by default so I would keep it like this.

mgmeiner commented 4 years ago

So think its complete now. I also added the integration test. It is quite difficult to check for exact the same config but i think its ok how its implemented now. Hope you're ok with that solution :)

wlezzar commented 4 years ago

I also added the integration test. It is quite difficult to check for exact the same config but i think its ok how its implemented now. Hope you're ok with that solution :)

I think what you did is exactly what's needed. No need to check the content of the config, it would just make the test flaky in the future