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

Add DescribeTopics and DescribeCluster support for admin client API #721

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

Conversation

alihan-synnada
Copy link

@alihan-synnada alihan-synnada commented Sep 13, 2024

Closes #614

Adding DescribeTopics now and also potentially DescribeCluster in this PR as per #614

I'll write the docs and tests, and then mark as ready for review

let high_bits = unsafe { rdsys::rd_kafka_Uuid_most_significant_bits(topic_id) } as u64;
let low_bits = unsafe { rdsys::rd_kafka_Uuid_least_significant_bits(topic_id) } as u64;
Uuid::from_u64_pair(high_bits, low_bits)
}
Copy link
Author

Choose a reason for hiding this comment

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

Is it really worth adding a dependency to the uuid crate when Uuid is only used here? We might as well remove the dependency and only return a (high_bits, low_bits) tuple.

@alihan-synnada alihan-synnada changed the title Add DescribeTopics support for admin client API Add DescribeTopics and DescribeCluster support for admin client API Sep 16, 2024
@alihan-synnada
Copy link
Author

I added DescribeCluster support as well. DescribeConsumerGroups should also be relatively easier to implement since the some of the types it returns (Node and AclOperation) are implemented now.

@alihan-synnada alihan-synnada marked this pull request as ready for review September 16, 2024 14:00
@alihan-synnada
Copy link
Author

I'm satisfied with the PR myself but there are a few points in my mind:

  • I'm not sure if the tests are covering enough ground.
  • I couldn't decide where to place structs and enums so they are somewhat all over the place.
  • Should we add the uuid crate as a dependency? The Uuid type is more descriptive and the uuid crate is rather lean when no features are enabled. On the other hand, it's only used for topic_id in TopicDescription so we can remove the dependency and simply use (u64, u64) or u128 instead.

@alihan-synnada
Copy link
Author

@davidblewett @benesch Any opportunities to review and possibly merge within next week?

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.

Add support for AdminAPI DescribeCluster() and DescribeTopics()
1 participant