KAFKA-7597: Make Trogdor ProduceBenchWorker support transactions#5885
KAFKA-7597: Make Trogdor ProduceBenchWorker support transactions#5885cmccabe merged 9 commits intoapache:trunkfrom stanislavkozlovski:KAFKA-7597-trogdor-transactions
Conversation
|
Retest this please |
|
Hmm. Just having a static "number of messages per transaction" field seems kind of limiting, right? What if we later want a mix of transactional and non-transactional messages, or transactions of different sizes? I would suggest making something like |
There was a problem hiding this comment.
nit: makes more sense as the singular TransactionAction than as the plural TransactionActions since each value represents a single action
There was a problem hiding this comment.
perhaps "zero" is better for a short name used in configuration? Although I guess most people will configure this by leaving it blank.
There was a problem hiding this comment.
It's probably better to make this synchronized, unless we want to say that TransactionActionGenerator classes are not thread-safe
There was a problem hiding this comment.
good catch
|
Looks good overall. I left some comments |
There was a problem hiding this comment.
Sorry to keep talking about the name of this, but I just don't like how TransactionActionGenerator sounds. TransactionGenerator sounds much better.
Then we could have TransactionGenerator#Action as well and it flows really well
|
Looks almost ready. Left some minor comments |
|
An unrelated test failed in JDK 8 -the dreaded |
There was a problem hiding this comment.
Can we name this transactionGenerator for simplicity?
There was a problem hiding this comment.
yep, good catch on the leftover
There was a problem hiding this comment.
If we use Optional<TransactionGenerator> we can get rid of INIT_TRANSACTIONS (it will be implied if the Optional contains a generator)
There was a problem hiding this comment.
The transactional ID should include produce-bench- in it somewhere to make it clear what test created this, right?
There was a problem hiding this comment.
Suggest a name like enableTransactions
There was a problem hiding this comment.
I thought about this a little bit more, and I think maybe we should just use Optional<TransactionGenerator> rather than having ZeroTransactionsGenerator. The zero generator isn't a real generator and it's kind of confusing
There was a problem hiding this comment.
This is a checkstyle issue. Are you compiling with check to catch these before Jenkins does?
There was a problem hiding this comment.
I think we should call this jacksonJdk8 or something.
There was a problem hiding this comment.
We should probably add this to other projects that depend on Jackson so that we get reasonable behaviour for Java 8 types.
It now accepts a new "messagesPerTransaction" field, which, if present, will enable transactional functionality in the bench worker. The producer will open N transactions with X messages each (bounded by the mandatory "maxMessages" field)
…on vars Imports new library - jackson-datatype-jdk8. This should be used until jackson 3 comes out, at which point we can switch to it completely Removed INIT_TRANSACTIONS state from the `TransactionAction` enum, this eases TransactionGenerator implementations Removed ZeroTransactionsGenerator class - it is much more intuitive to provide null or not define the transactionsGenerator class
| * This means that most of the time it should return #{@link TransactionAction#NO_OP} | ||
| * to signal the producer that its next step should be to send a message. | ||
| */ | ||
| TransactionAction action(); |
There was a problem hiding this comment.
nextAction might be a better name here
…d var, rename transactional id
| new SequentialPayloadGenerator(4, 0) : keyGenerator; | ||
| this.valueGenerator = valueGenerator == null ? | ||
| new ConstantPayloadGenerator(512, new byte[0]) : valueGenerator; | ||
| this.transactionGenerator = txGenerator; |
There was a problem hiding this comment.
We should convert null -> none here. Even if Jackson always fills this in, someone may manually create this object
|
LGTM |
KAFKA-7597: Add configurable transaction support to ProduceBenchWorker. In order to get support for serializing Optional<> types to JSON, add a new library: jackson-datatype-jdk8. Once Jackson 3 comes out, this library will not be needed. Reviewers: Colin McCabe <cmccabe@apache.org>, Ismael Juma <ismael@juma.me.uk>
3 participants
It now accepts a new "messagesPerTransaction" field, which, if present, will enable transactional functionality in the bench worker.
The producer will open N transactions with X messages each (bounded by the mandatory "maxMessages" field)