You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/05/08 05:59:31 UTC
[GitHub] [kafka] abbccdda opened a new pull request #8632: KAFKA-9972: Only commit tasks with valid states
abbccdda opened a new pull request #8632:
URL: https://github.com/apache/kafka/pull/8632
We spotted a case in the soak test where a standby task could be in `CREATED` state during commit, which causes an illegal state exception. To prevent this from happening, the solution is to always enforce a state check.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on pull request #8632: KAFKA-9972: Only commit tasks with valid states
Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #8632:
URL: https://github.com/apache/kafka/pull/8632#issuecomment-625653951
Retest this please.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] abbccdda commented on pull request #8632: KAFKA-9972: Only commit tasks with valid states
Posted by GitBox <gi...@apache.org>.
abbccdda commented on pull request #8632:
URL: https://github.com/apache/kafka/pull/8632#issuecomment-625663935
Hmm, it won't pass as I have already spotted some unit test failures, will fix them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] abbccdda commented on pull request #8632: KAFKA-9972: Only commit tasks with valid states
Posted by GitBox <gi...@apache.org>.
abbccdda commented on pull request #8632:
URL: https://github.com/apache/kafka/pull/8632#issuecomment-626205161
Got 2/3 green build with only one test failure:
```
org.apache.kafka.streams.integration.EosBetaUpgradeIntegrationTest.shouldUpgradeFromEosAlphaToEosBeta[false]
java.lang.AssertionError: Condition not met within timeout 60000. Clients did not startup and stabilize on time. Observed transitions:
client-1 transitions: [KeyValue(RUNNING, REBALANCING), KeyValue(REBALANCING, RUNNING)]
client-2 transitions: [KeyValue(CREATED, REBALANCING), KeyValue(REBALANCING, RUNNING), KeyValue(RUNNING, REBALANCING), KeyValue(REBALANCING, RUNNING)]
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on pull request #8632: KAFKA-9972: Only commit tasks with valid states
Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #8632:
URL: https://github.com/apache/kafka/pull/8632#issuecomment-625948915
Retest this please.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on pull request #8632: KAFKA-9972: Only commit tasks with valid states
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on pull request #8632:
URL: https://github.com/apache/kafka/pull/8632#issuecomment-626078933
Sure thing, that doesn't need to block this PR. As long as it doesn't slip off the radar completely I'm 👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei commented on a change in pull request #8632: KAFKA-9972: Only commit tasks with valid states
Posted by GitBox <gi...@apache.org>.
vvcephei commented on a change in pull request #8632:
URL: https://github.com/apache/kafka/pull/8632#discussion_r422329885
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/TaskManager.java
##########
@@ -732,15 +732,16 @@ Task taskForInputPartition(final TopicPartition partition) {
return tasks.values().stream().filter(t -> !t.isActive());
}
+ // For testing only.
+ int commitAll() {
+ return commit(tasks.values());
+ }
+
Review comment:
FWIW, I'd rather just inline it and avoid maintaining APIs that are just for tests in our production code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on pull request #8632: KAFKA-9972: Only commit tasks with valid states
Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #8632:
URL: https://github.com/apache/kafka/pull/8632#issuecomment-626074854
Retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on pull request #8632: KAFKA-9972: Only commit tasks with valid states
Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #8632:
URL: https://github.com/apache/kafka/pull/8632#issuecomment-626001877
Retest this please.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on pull request #8632: KAFKA-9972: Only commit tasks with valid states
Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #8632:
URL: https://github.com/apache/kafka/pull/8632#issuecomment-626074819
Java 8 passed.
Java 11 and 14 failed. (Test results are not longer available.)
@ableegoldman You raise a good point. Can we revisit this when doing the kip-447 follow up refactoring? I would like to merge this PR to we can re-deploy soak with the fix?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] abbccdda commented on a change in pull request #8632: KAFKA-9972: Only commit tasks with valid states
Posted by GitBox <gi...@apache.org>.
abbccdda commented on a change in pull request #8632:
URL: https://github.com/apache/kafka/pull/8632#discussion_r422332367
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/TaskManager.java
##########
@@ -732,15 +732,16 @@ Task taskForInputPartition(final TopicPartition partition) {
return tasks.values().stream().filter(t -> !t.isActive());
}
+ // For testing only.
+ int commitAll() {
+ return commit(tasks.values());
+ }
+
Review comment:
I see your point and thought about it, but I feel a little bit reluctant to remove it for the reason that it is indeed a handy call to make for testing purpose :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on pull request #8632: KAFKA-9972: Only commit tasks with valid states
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on pull request #8632:
URL: https://github.com/apache/kafka/pull/8632#issuecomment-626028839
What about all the other times we commit tasks in TaskManager? Aren't we still vulnerable to committing a CREATED task during `handleAssignment`, for example? Imho it makes sense to push this logic into the StreamTask/StandbyTask rather than chase down all code paths that may commit and make sure we are passing in only "valid" tasks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org