EoS TxnOffsetCommit v0 #195
Conversation
ianwsperber
force-pushed
the
eos-169-TxnOffsetCommit-v0
branch
from
71077b4
to
4251127
Compare
There was a problem hiding this comment.
In testing I was not able to get a rejection for an invalid transaction id/producer id/producer epoch
Interesting, looking at the Kafka code I'm assuming that since you haven't followed the "protocol" (init, add partitions, etc) it end up in a state where it can't validate the transaction. Can you try a version where you call the previous methods first? Maybe this was fixed on the newer versions
src/broker/index.js
Outdated
| * @param {string} groupId The unique group identifier (for the consumer group) | ||
| * @param {number} producerId Current producer id in use by the transactional id. | ||
| * @param {number} producerEpoch Current epoch associated with the producer id. | ||
| * @param {object[]} topics |
There was a problem hiding this comment.
What about
/**
* @param {OffsetCommitTopic[]} topics
*
* @typedef {Object} OffsetCommitTopic
* @property {string} topic
* @property {OffsetCommitTopicPartition[]} partitions
*
* @typedef {Object} OffsetCommitTopicPartition
* @property {number} partition
* @property {number} offset
* @property {string} metadata
*/For the topics JSDoc?
| producerId = result.producerId | ||
| producerEpoch = result.producerEpoch | ||
|
|
||
| await transactionBroker.addPartitionsToTxn({ |
There was a problem hiding this comment.
@tulios I've added the preceding steps in the protocol here. You can see below we still don't get a rejection for invalid values.
There was a problem hiding this comment.
Yeah, well that's unfortunate, but I guess we will have more data once we start the actual producer. If we discover something new, we can come back add more tests.
There was a problem hiding this comment.
Sounds good to me @tulios. If we ultimately conclude that this is indeed Kafka's behavior today it may be prudent to raise an issue with the Kafka team to identify whether it's the intended behavior or a bug.
|
@ianwsperber I was trying to test the commit flow following the steps in here and I just realized that we are not passing the transaction attributes to the produce request, I started some of this work here: https://github.com/tulios/kafkajs/compare/add-transactional-attrs-to-record-batch I have a theory that we do not see the correct behavior because we haven't produced any messages, so there's nothing associated with the I'm busy with some other things this week and the next, but I should be able to join this initiative soon. |
Resolves #169.
In testing I was not able to get a rejection for an invalid transaction id/producer id/producer epoch. Accordingly I wrote my broker test to just use mock values for these fields to make that behavior explicit. @tulios do you have any thoughts on this? According to the design doc my request should be rejected, but I'm unsure if that's Kafka's actual behavior.
Also added a little bit of documentation to the transaction methods in the broker.