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

Admin protocol updates #1948

Merged
merged 11 commits into from
Dec 29, 2019
Merged

Admin protocol updates #1948

merged 11 commits into from
Dec 29, 2019

Conversation

TylerLubeck
Copy link
Contributor

@TylerLubeck TylerLubeck commented Nov 8, 2019

I've been digging in to the admin client and found myself adding extra api version support


This change is Reviewable

('config_names', String('utf-8')),
('config_value', String('utf-8')),
('read_only', Boolean),
('config_source', Int8),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like this was renamed from is_default to config_source, and the type changed from Boolean to Int8 in v1 as well, but I didn't include that fix here. Should I?

Copy link
Owner

Choose a reason for hiding this comment

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

Ugh. I do hate to make breaking changes... But I could go either way on this one.

@TylerLubeck TylerLubeck force-pushed the admin-protocol-updates branch from c51d814 to 68d8aad Compare November 8, 2019 23:13
@TylerLubeck TylerLubeck force-pushed the admin-protocol-updates branch from 68d8aad to fa35d52 Compare November 8, 2019 23:23
@TylerLubeck
Copy link
Contributor Author

I know how this sounds, but I can't for the life of me reproduce the test failure locally. Any suggestions?

Tyler Lubeck added 2 commits November 15, 2019 10:24
python > 3.3 will error at runtime so while this test is extraneous
it'll help catch typos like this
@dpkp
Copy link
Owner

dpkp commented Dec 29, 2019

Test failure is known to be flakey and has been marked as xfail in master (see #1929)

Copy link
Owner

@dpkp dpkp left a comment

Choose a reason for hiding this comment

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

Looks great! I haven't been following the latest protocol changes... why are there so many version bumps with no changes to request/response schemas?

('config_names', String('utf-8')),
('config_value', String('utf-8')),
('read_only', Boolean),
('config_source', Int8),
Copy link
Owner

Choose a reason for hiding this comment

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

Ugh. I do hate to make breaking changes... But I could go either way on this one.

@@ -988,6 +999,9 @@ def describe_consumer_groups(self, group_ids, group_coordinator_id=None):
useful for avoiding extra network round trips if you already know
the group coordinator. This is only useful when all the group_ids
have the same coordinator, otherwise it will error. Default: None.
:param include_authorized_operatoins: Whether or not to include
Copy link
Owner

Choose a reason for hiding this comment

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

sp: operations (operatoins)

@dpkp dpkp merged commit e06ea70 into dpkp:master Dec 29, 2019
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