KAFKA-4923: Add Exaclty-Once Semantics to Streams (testing)#2974
Conversation
mjsax
commented
May 4, 2017
mjsax
commented
- add broker compatibility system tests
- add broker compatibility system tests
|
Call for review @enothereska @dguy |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
| <module name="BooleanExpressionComplexity"> | ||
| <!-- default is 3 --> | ||
| <property name="max" value="4"/> | ||
| <property name="max" value="5"/> |
There was a problem hiding this comment.
Can you just add a suppression for the file that needs to have 5 booleans? There is little point in having rules if people are just going to keep on changing the values
There was a problem hiding this comment.
There is also little point in adding an exception for each single file that break the rule IMHO. I just think that 4 (and even 5) is a rather small number anyway. (But this goes back to you other comment -- if we only check a single API key, we could keep 4 here.)
There was a problem hiding this comment.
Hmmm. well the point is to not break the rule! If you really feel you need to break the rule then add a suppression
| } | ||
|
|
||
| private boolean brokerSupportsTransactions(final ApiVersionsResponse apiVersionsResponse) { | ||
| return apiVersionsResponse.apiVersion(ApiKeys.INIT_PRODUCER_ID.id) != null |
There was a problem hiding this comment.
Do we actually need to check for all of these ApiKeys? i.e., we know that they are all going in as part of the same release, can we not just check for at least that version instead?
There was a problem hiding this comment.
Don't think there is a strict need -- I thought it might be "cleaner" to check all. But maybe that overkill. Curious what others think about this.
There was a problem hiding this comment.
I guess it doesn't harm.
There was a problem hiding this comment.
Agree that it is safer to check all: though it may unlike to happen but we may remove some of the APIs moving forward.
|
System tests passed: https://jenkins.confluent.io/job/system-test-kafka-branch-builder-2/288/ |
|
What kind of testing is this PR supposed to cover (so I know if it's complete or not)? |
|
It adds:
The whole system test should cover: https://github.com/apache/kafka/pull/2974/files#diff-7924cd3d80f77980ae421699e631e2ccR29 |
dguy
left a comment
dguy
left a comment
There was a problem hiding this comment.
Comments were only minor. LGTM
|
@guozhangwang Call for review and merging. |
|
Refer to this link for build results (access rights to CI server needed): |
| } | ||
|
|
||
| private boolean brokerSupportsTransactions(final ApiVersionsResponse apiVersionsResponse) { | ||
| return apiVersionsResponse.apiVersion(ApiKeys.INIT_PRODUCER_ID.id) != null |
There was a problem hiding this comment.
Agree that it is safer to check all: though it may unlike to happen but we may remove some of the APIs moving forward.
…treams - add broker compatibility system tests Author: Matthias J. Sax <matthias@confluent.io> Reviewers: Damian Guy, Eno Thereska, Guozhang Wang Closes #2974 from mjsax/kafka-4923-add-eos-to-streams-add-broker-check-and-system-test (cherry picked from commit 495836a) Signed-off-by: Guozhang Wang <wangguoz@gmail.com>
|
Merged to trunk and 0.11.0. |
5 participants
