MINOR: Cleanup handling of mixed transactional/idempotent records#6172
Conversation
|
|
||
| @Test | ||
| public void testMixedTransactionalData() { | ||
| long producerId = 15L; |
There was a problem hiding this comment.
can we make some / all of these final ?
|
Thanks, @hachikuji . I left a comment. LGTM |
|
@cmccabe Pushed a commit. I will merge after the build completes. Thanks for reviewing! |
| if (produceRequest.isTransactional) { | ||
| if (!authorize(request.session, Write, Resource(TransactionalId, produceRequest.transactionalId, LITERAL))) { | ||
| if (produceRequest.hasTransactionalRecords) { | ||
| val isAuthorizedTransactional = produceRequest.transactionalId != null && |
There was a problem hiding this comment.
What's the intended behaviour if transactionalId == null && produceRequest.hasTransactionalRecords?
There was a problem hiding this comment.
Returning TRANSACTIONAL_ID_AUTHORIZATION_FAILED seemed like the best bet. INVALID_REQUEST might be another option. Currently, without this check, the authorizer raises an NPE which causes the connection to terminate. We could push the null checks into the authorizor as well.
There was a problem hiding this comment.
Makes sense. I guess one can say that the null transaction id is never authorized.
rajinisivaram
left a comment
rajinisivaram
left a comment
There was a problem hiding this comment.
@hachikuji Thanks for the PR, LGTM
…ache#6172) Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com>, Ismael Juma <ismael@juma.me.uk>, Colin Patrick McCabe <colin@cmccabe.xyz>
| return transactionalId; | ||
| } | ||
|
|
||
| public boolean isTransactional() { |
There was a problem hiding this comment.
fwiw, the renamed methods are a breaking change of public API and shouldn't have been merged into master as is, I think. Why not just @deprecate them? Asking, because this has caused me some headache today :(
There was a problem hiding this comment.
The request classes are not public API. Only the classes shown in the javadoc are considered public:
https://kafka.apache.org/21/javadoc/overview-summary.html
Out of curiosity, how are you using this internal classes?
There was a problem hiding this comment.
Thanks for getting back to me. We're using Alpakka Kafka and I wanted to test the client-version 2.1.1 talking with server 2.0.1.
For testing purposes we're spinning up an embedded Kafka, which comes from a transitive dependency that akka-stream-kafka-testkit provides. These APIs are used somewhere in there, so it's not even code that I control.
I wasn't aware that this is considered "private public" API and was a bit worried about the "unnoticed" breaking changes to the API. Thanks for the explanation
* ak/trunk: MINOR: fix race condition in KafkaStreamsTest (apache#6185) KAFKA-4850: Enable bloomfilters (apache#6012) MINOR: ducker-ak: add down -f, avoid using a terminal in ducker test KAFKA-5117: Stop resolving externalized configs in Connect REST API MINOR: Cleanup handling of mixed transactional/idempotent records (apache#6172) KAFKA-7844: Use regular subproject for generator to fix *All targets (apache#6182) Fix Documentation for cleanup.policy is out of date (apache#6181) MINOR: increase timeouts for KafkaStreamsTest (apache#6178) MINOR: Rejoin split ssl principal mapping rules (apache#6099) MINOR: Handle case where connector status endpoints returns 404 (apache#6176) MINOR: Remove unused imports, exceptions, and values (apache#6117) KAFKA-3522: Add internal RecordConverter interface (apache#6150) Fix Javadoc of KafkaConsumer (apache#6155) KAFKA-6455: Extend CacheFlushListener to forward timestamp (apache#6147) MINOR: Log partition info when creating new request batch in controller (apache#6145) KAFKA-7652: Part I; Fix SessionStore's findSession(single-key) (apache#6134) MINOR: Remove the InvalidTopicException handling in InternalTopicManager (apache#6167) [KAFKA-7024] Rocksdb state directory should be created before opening the DB (apache#6138) MINOR:: Fix typos (apache#6079)
…ache#6172) Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com>, Ismael Juma <ismael@juma.me.uk>, Colin Patrick McCabe <colin@cmccabe.xyz>
5 participants
Committer Checklist (excluded from commit message)