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

produce_batch() does not honour per-message partition #1604

Closed
edenhill opened this issue Dec 23, 2017 · 8 comments
Closed

produce_batch() does not honour per-message partition #1604

edenhill opened this issue Dec 23, 2017 · 8 comments

Comments

@edenhill
Copy link
Contributor

edenhill commented Dec 23, 2017

Will need to add a new msgflag to tell it to use per-message partition.
Found in confluentinc/confluent-kafka-go#112

Note: It is likely that applications are default-initializing the partition to 0, not -1 (UA).

@barrotsteindev
Copy link
Contributor

Hey @edenhill,
I'd love to start working on this issue.
Do you have any suggestions?

@edenhill
Copy link
Contributor Author

edenhill commented Jan 2, 2018

@barrotsteindev Great!

Four things needs to happen:

  • add a test case in tests/0011-produce_batch.c - the test should fail prior to the fix.
  • add a ..MSG_F_PARTITION flag (which will be passed in msgflags) to rdkafka.h
  • produce_batch() needs to be reworked to support per-message partition if _MSG_F_PARTITION is set.
  • add documentation (to produce_batch() in rdkafka.h) for this new behaviour.

I suggest you base your PR off of retry_fixes which has changes to produce_batch()

@barrotsteindev
Copy link
Contributor

@edenhill,
I'm having trouble getting the partition count of the topic in 0011-produce_batch.c file.
https://github.com/barrotsteindev/librdkafka/blob/2713961bc1c610d02f34cef420525324381be00b/tests/0011-produce_batch.c#L229
how can i get the complete definition for the struct type so i can access its fields?
thanks in advance

@edenhill
Copy link
Contributor Author

edenhill commented Jan 6, 2018

These tests rely on auto.create.topics.enable=true and num.partitions=4 on the broker.
If you need something more specific you can use test_create_topic()

@edenhill
Copy link
Contributor Author

edenhill commented Jan 6, 2018

I suggest you create a new test (within 0011-..c, look at the bottom) not to interfere with existing tests (since that would make it hard to catch regression)

@barrotsteindev
Copy link
Contributor

4 seems like enough.
Is it OK for me to assume there are 4 partitions?
since i count the number of messages i assigned to each partition and using the callback i compare the counts.
I have create a new test, test_per_message_partition_flag.
I have a few more finishing touches and I will create a PR.

@edenhill
Copy link
Contributor Author

edenhill commented Jan 6, 2018

I think it might be better to explicitly create the topic with test_create_topic() to make the test future proof.

@barrotsteindev
Copy link
Contributor

Thanks for the guidance,
I have now implemented the test using test_create_topic().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants