Add KafkaAdmin class #1540

llamahunter · 2018-07-12T18:45:02Z

Requires cluster version > 0.10.0.0, and uses new wire protocol
classes to do many things via broker connection that previously
needed to be done directly in zookeeper.

This change is

llamahunter · 2018-07-12T18:47:54Z

Not really a python pro, so please provide feedback on conventions, etc. Also, not really sure how to do extensive unit testing. The existing tests seem very integration-centric, and don't actually run successfully for me on my machine.

dpkp · 2018-07-14T15:39:33Z

This is great work! Thanks so much for submitting. My only top-level question is how do you think we should handle api version selection in a multi-version cluster? I.e., during a cluster upgrade. In the current non-admin case we provide a way for users to pass in an explicit "broker version". A more robust approach might be to always check the version of the broker we are sending the request to, but that gets significantly more complicated. Finally, maybe the last option is to advertise in the documentation that it is not safe to use while the cluster is in mixed-version / rolling upgrade state. What do you think?

llamahunter · 2018-07-14T19:35:05Z

Well, I'm using the ApiVersions data structure returned by the broker to select the max version that both the library and the broker supports for each individual API call. I think hard coding that "0.11.0.2 means XYZ parameters on operations ABC are supported" is error prone, especially when the brokers already provide us with exactly what versions of each API call they support.

I think that if someone is doing an admin operation on a mixed version cluster, they should be sure that they don't use any of the optional parameters that aren't supported by all brokers in the cluster. Tho, I think that most (all?) admin calls go to the cluster controller, so it really only matters what that broker version is at.

llamahunter · 2018-07-16T16:40:36Z

Hmm. I was fetching the ApiVersions map from whatever happened to be the 'least loaded node', but that's not necessarily the controller. All the admin calls go to the controller, so really need to check what ApiVersions it supports. Moved the check_version() call there.

llamahunter · 2018-07-16T16:48:01Z

So, looking for some feedback on whether I should augment the API with return types for the calls. Right now, it's sort of just delivering back whatever Kafka wire protocol object. But, it looks like that's only got python attributes for the top level, and subobjects are just tuples or something, making them difficult to use. Thoughts? Is there a way to make the subobjects returned by the Schema parser have attributes? Should I unmarshal into typed python objects?

dpkp

This is great! Only issue is the enum requirement for py27. We should address that before merging.

Re: object types, that's a larger issue that we can leave for a separate PR. But you are exactly right -- making the response classes easier to work with directly would be very useful

dpkp · 2018-08-31T13:48:39Z

kafka/admin/config_resource.py

@@ -0,0 +1,31 @@
+from __future__ import absolute_import
+
+from enum import IntEnum


We have a pretty strict rule against mandatory external dependencies. I know this is standard in py3, but it isn't in py2. Can we avoid using it? If not, we should vendor the py27 version (see kafka/vendor)

ugh... people are still using python 2? Isn't it EOL? Hrmph... lemme look at it.

Any progress @llamahunter ?

And yes, I know of multiple companies who heavily rely on this library and still have large codebases in python 2. Most are planning to migrate to python 3, but still in the early stages. I'd guess it'll be another 2-3 years before I'd be okay with saying that we as a core infra library are going to stop supporting python 2 in our releases. I look forward to that day, it's just not quite here yet.

FWIW, I'd love to see this be part of the project as well.

I added vendoring of enum34 in #1604. To me, since enum is in the python 3 stdlib, it makes more sense to vendor it rather than to workaround it. That way we can start using it in other code as well. And we won't always be stuck with this because whenever we drop python 2 support we can stop vendoring it.

This is needed for #1540 While the usage there is trivial and could probably be worked around, I'd rather vendor it so that future code can use enums... since `enum` is already available in the python 3 stdlib, this will be easy enough to eventually stop vendoring whenever we finally drop python 2 support.

llamahunter · 2018-10-22T16:05:49Z

Sorry about the delay. Will look into this today. Still would like feedback about the return format. Should there be structured data structures rather than the loosely typed property bags? Is there some better way of using the Schema class to make the properties easier to access?

This is needed for #1540 While the usage there is trivial and could probably be worked around, I'd rather vendor it so that future code can use enums... since `enum` is already available in the python 3 stdlib, this will be easy enough to eventually stop vendoring whenever we finally drop python 2 support.

jeffwidman · 2018-10-22T21:38:58Z

I just added enum34 in #1604--use it as a fallback, something like this:

# enum in stdlib as of py3.4
try:
    import enum  # pylint: disable=import-error
except ImportError:
    # vendored backport module
    from kafka.vendor import enum34 as enum

Re: the schema stuff--good question that I haven't had time to look into yet... @dpkp / @tvoinarovskyi your thoughts?

dpkp

great work! I think the structured data for Request/Response etc is an area ripe for improvement wrt property access. When I first put Struct/Schema together I left that as an exercise for later. Feel free to improve now or just ship it as is.

jeffwidman · 2018-10-23T03:42:07Z

tox.ini

@@ -21,6 +21,7 @@ deps =
    crc32c
    py26: unittest2
    decorator
+    py27: enum34


since this is vendored, no need to include this

jeffwidman · 2018-10-23T03:48:32Z

@llamahunter let's just ship as is for now to land the existing changes and then leave figuring out a cleaner design for a later PR... I would not be surprised if we get a few bug reports about this, so I'd rather get it live in the hands of users on master sooner rather than later, even if the interface changes slightly in a later release.

Besides, with the 2.0 release impending once I get #1196, it will be an convenient place to do a breaking change in this interface if needed.

So if you can rebase and use the vendored enum34 when enum is not available, than I think that's all that is needed to get this merged.

jeffwidman · 2018-10-23T22:26:10Z

I'd like to land this, as I have some additional experiments I'd like to run on top of it, so I just rebased to fix conflicts and also added the vendored enum34 import... I think we will probably have at least one more 1.4.x bugfix release, and for those we can include this with the public interface marked as experimental...

So as soon as travis gives the green light I'm going to merge.

llamahunter · 2018-10-23T22:29:58Z

Ah, sorry if I conflicted. I rebased and removed the tox.ini change. Had some weird local problems running the tests, tho. Hope it works on travis.

jeffwidman · 2018-10-23T22:53:09Z

No worries... I'd forgotten about the tox.ini change, and it looks like you forgot to fallback to enum34 on python 2, so I just updated it to pull in both changes.

I'm going to force-push over the top of your change. Also, for future reference, you can do git push --force-with-lease and you'll be notified if someone else changed the tip already.

jeffwidman · 2018-10-23T23:12:42Z

Also, I think the test failures are my fault from #1605 so I will get those fixed in a separate PR and then merge this...

Github was down, so travis wasn't running the tests correctly and I didn't realize it til now

llamahunter · 2018-10-24T16:51:34Z

Ok... so, what needs doing here? Do I need to add

# enum in stdlib as of py3.4
try:
    import enum  # pylint: disable=import-error
except ImportError:
    # vendored backport module
    from kafka.vendor import enum34 as enum

for my use of IntEnum type?

jeffwidman · 2018-10-24T20:18:28Z

No, I already got that change in there. I was waiting until I had a chance to fix the pylint issue which was breaking tests... that is handled in #1609 / 0d766cf, so I just rebased and am just waiting for tests to pass and then I will merge this.

Requires cluster version > 0.10.0.0, and uses new wire protocol classes to do many things via broker connection that previously needed to be done directly in zookeeper.

jeffwidman · 2018-10-25T05:42:34Z

Tests took forever to get green due to repeated flakiness upstream of travis--failed downloads etc.

But now we finally have this in! Thank you again. 🍰

llamahunter mentioned this pull request Jul 12, 2018

Create AdminClient #935

Closed

llamahunter force-pushed the add-admin-client branch 7 times, most recently from b046d25 to a6c7a2b Compare July 13, 2018 21:26

dpkp requested changes Aug 31, 2018

View reviewed changes

This was referenced Oct 22, 2018

Integration tests migration to pytest framework #1293

Merged

Vendor enum34 #1604

Merged

dpkp approved these changes Oct 23, 2018

View reviewed changes

jeffwidman reviewed Oct 23, 2018

View reviewed changes

jeffwidman force-pushed the add-admin-client branch from c67a9ab to c58fcbf Compare October 23, 2018 22:24

llamahunter force-pushed the add-admin-client branch from c58fcbf to 8076552 Compare October 23, 2018 22:28

jeffwidman force-pushed the add-admin-client branch from 8076552 to 6a88ec0 Compare October 23, 2018 22:53

jeffwidman force-pushed the add-admin-client branch from 6a88ec0 to 5afdcbe Compare October 23, 2018 23:00

Add KafkaAdmin class

71f748e

Requires cluster version > 0.10.0.0, and uses new wire protocol classes to do many things via broker connection that previously needed to be done directly in zookeeper.

jeffwidman force-pushed the add-admin-client branch from 5afdcbe to 71f748e Compare October 24, 2018 20:19

jeffwidman merged commit 481f880 into dpkp:master Oct 25, 2018

jeffwidman mentioned this pull request Oct 25, 2018

Add CreateTopics support to kafka-python's AdminClient #1104

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add KafkaAdmin class #1540

Add KafkaAdmin class #1540

llamahunter commented Jul 12, 2018 •

edited by dpkp

Loading

llamahunter commented Jul 12, 2018

dpkp commented Jul 14, 2018

llamahunter commented Jul 14, 2018

llamahunter commented Jul 16, 2018

llamahunter commented Jul 16, 2018

dpkp left a comment

dpkp Aug 31, 2018

llamahunter Aug 31, 2018

jeffwidman Sep 27, 2018

sbreidbach-edr Oct 18, 2018

jeffwidman Oct 22, 2018

llamahunter commented Oct 22, 2018

jeffwidman commented Oct 22, 2018 •

edited

Loading

dpkp left a comment •

edited

Loading

jeffwidman Oct 23, 2018

jeffwidman commented Oct 23, 2018

jeffwidman commented Oct 23, 2018 •

edited

Loading

llamahunter commented Oct 23, 2018

jeffwidman commented Oct 23, 2018

jeffwidman commented Oct 23, 2018

llamahunter commented Oct 24, 2018

jeffwidman commented Oct 24, 2018

jeffwidman commented Oct 25, 2018 •

edited

Loading

		@@ -0,0 +1,31 @@
		from __future__ import absolute_import

		from enum import IntEnum

Add KafkaAdmin class #1540

Add KafkaAdmin class #1540

Conversation

llamahunter commented Jul 12, 2018 • edited by dpkp Loading

llamahunter commented Jul 12, 2018

dpkp commented Jul 14, 2018

llamahunter commented Jul 14, 2018

llamahunter commented Jul 16, 2018

llamahunter commented Jul 16, 2018

dpkp left a comment

Choose a reason for hiding this comment

dpkp Aug 31, 2018

Choose a reason for hiding this comment

llamahunter Aug 31, 2018

Choose a reason for hiding this comment

jeffwidman Sep 27, 2018

Choose a reason for hiding this comment

sbreidbach-edr Oct 18, 2018

Choose a reason for hiding this comment

jeffwidman Oct 22, 2018

Choose a reason for hiding this comment

llamahunter commented Oct 22, 2018

jeffwidman commented Oct 22, 2018 • edited Loading

dpkp left a comment • edited Loading

Choose a reason for hiding this comment

jeffwidman Oct 23, 2018

Choose a reason for hiding this comment

jeffwidman commented Oct 23, 2018

jeffwidman commented Oct 23, 2018 • edited Loading

llamahunter commented Oct 23, 2018

jeffwidman commented Oct 23, 2018

jeffwidman commented Oct 23, 2018

llamahunter commented Oct 24, 2018

jeffwidman commented Oct 24, 2018

jeffwidman commented Oct 25, 2018 • edited Loading

llamahunter commented Jul 12, 2018 •

edited by dpkp

Loading

jeffwidman commented Oct 22, 2018 •

edited

Loading

dpkp left a comment •

edited

Loading

jeffwidman commented Oct 23, 2018 •

edited

Loading

jeffwidman commented Oct 25, 2018 •

edited

Loading