-
Notifications
You must be signed in to change notification settings - Fork 910
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
[KIP-396] List offsets implementation #1576
Conversation
ca13c4e
to
7f2d748
Compare
c483723
to
767ea7e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are the comments, sent a PR where I've done most of the suggestions for testing them.
and move them to a separate file
…fsets-changes [KIP-396] Address most of the PR comments
and _make_generic_futmap_result
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First Round of Review. Checking test cases.
examples/get_watermark_offsets.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we specify that the list_offsets
example is present in adminapi.py
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not in the changed files we can add a comment but do not think that is a great thing to do, list_offsets is admin api anybody can easily search it. So seems unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mahajanadhitya @pranavrth . This looks good, let's wait for @pranavrth approval or for other comments.
examples/adminapi.py
Outdated
topic_partition_offsets = {} | ||
if len(args) < 1: | ||
raise ValueError( | ||
f"Invalid number of arguments for list offsets, expected at least 1, got {len(args)}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it can only be 0 here no? len(args) < 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few more comments.
Add Changelog.
an empty future dictionary. Increased test coverage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!. Minor nits.
No description provided.