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.

Sign up for GitHub

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

Jump to bottom

Support rd_kafka_memberid() from python clients #1154

Closed
wants to merge 5 commits into from
Closed

Support rd_kafka_memberid() from python clients #1154

wants to merge 5 commits into from

Conversation

jliunyu
Copy link
Contributor

@jliunyu jliunyu commented Jun 29, 2021 •
edited
Loading

librdkafka has a rd_kafka_memberid() function, but it is not exposed in the Python client yet.

Local test:

tests/test_Consumer.py::test_consumer_without_groupid PASSED
tests/test_Consumer.py::test_consumer_memberid Jing Liu before partitioner
Jing Liu no partitioner
msg after consumer b'Hello Go!'
Jing Liu memberid start librdkafkaJing Liu memberid librdkafka rdkafka-29c884cd-cca3-4a18-a62f-e6ddd8bcd8ea
jing liu after poll memberid rdkafka-29c884cd-cca3-4a18-a62f-e6ddd8bcd8ea
PASSED

@jliunyu jliunyu requested a review from edenhill June 29, 2021 23:06
@jliunyu jliunyu linked an issue Jun 29, 2021 that may be closed by this pull request
Question: how to get the consumer id assigned by Kafka? #1139
Closed
edenhill
edenhill suggested changes Jun 30, 2021
View reviewed changes
Copy link
Contributor

@edenhill edenhill left a comment

Choose a reason for hiding this comment

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

Good stuff, needs some changes. Don't forget the CHANGELOG (Enhancement)

src/confluent_kafka/src/Consumer.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Consumer.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Consumer.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Consumer.c Show resolved Hide resolved
src/confluent_kafka/src/Consumer.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Consumer.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Consumer.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Consumer.c Outdated Show resolved Hide resolved
tests/integration/consumer/test_consumer_memberid.py Outdated Show resolved Hide resolved
tests/integration/consumer/test_consumer_memberid.py Show resolved Hide resolved
edenhill
edenhill suggested changes Jul 7, 2021
View reviewed changes
Copy link
Contributor

@edenhill edenhill left a comment

Choose a reason for hiding this comment

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

Almost there! Also add a CHANGELOG entry in the enhancement section

src/confluent_kafka/src/Consumer.c Outdated Show resolved Hide resolved
tests/integration/consumer/test_consumer_memberid.py Show resolved Hide resolved
tests/integration/consumer/test_consumer_memberid.py Show resolved Hide resolved
src/confluent_kafka/src/Consumer.c Outdated Show resolved Hide resolved
edenhill
edenhill suggested changes Jul 12, 2021
View reviewed changes
CHANGELOG.md

### Enhancements

- Support rd_kafka_memberid() from python clients (#1154).
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't expose the librdkafka name here, but the python one, so something like:
- Added consumer.memberid() method to retrieve the consumer's group member id (#1154)

tests/integration/consumer/test_consumer_memberid.py
consumer = kafka_cluster.consumer(consumer_conf)

assert consumer is not None
assert len(consumer.memberid()) == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should check that the memberid is None rather than have a 0 length (which could be an empty string or some other type).

Copy link
Contributor Author

@jliunyu jliunyu Jul 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hi Magnus, thanks for reviewing.

I found that the consumer.memberid() is "" here when I do the test. But do you think None should be the expected result or "" is also Okay?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. I think it should be None as that seems more Pythonic.

edenhill
edenhill suggested changes Sep 20, 2021
View reviewed changes
tests/integration/consumer/test_consumer_memberid.py
consumer = kafka_cluster.consumer(consumer_conf)

assert consumer is not None
assert len(consumer.memberid()) == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. I think it should be None as that seems more Pythonic.

@markns
Copy link

markns commented Dec 1, 2021

Hi @jliunyu any chance this pr could make it into the 1.8.0 release? It would be quite handy for us. Thx

@pranavrth pranavrth mentioned this pull request Oct 27, 2022
Merged
pranavrth added a commit that referenced this pull request Nov 2, 2022
@pranavrth
CLIENTS-1765: Added support for consumer.memberid() #1154 (#1455)
395f939 
Added support for consumer.memberid()
@pranavrth
Copy link
Member

Created and merged a new PR for these changes - #1455

Closing this one.

@pranavrth pranavrth closed this Nov 2, 2022
mahajanadhitya pushed a commit that referenced this pull request Feb 3, 2023
@pranavrth @mahajanadhitya
CLIENTS-1765: Added support for consumer.memberid() #1154 (#1455)
05bed0b 
Added support for consumer.memberid()
emasab pushed a commit that referenced this pull request Jun 19, 2023
@pranavrth @emasab
CLIENTS-1765: Added support for consumer.memberid() #1154 (#1455)
fc2e3c8 
Added support for consumer.memberid()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edenhill edenhill edenhill requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Question: how to get the consumer id assigned by Kafka?
4 participants
@jliunyu @markns @pranavrth @edenhill