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 2021/06/19 13:53:40 UTC
[GitHub] [kafka] chia7712 opened a new pull request #10908: MINOR: fix round_trip_fault_test.py - don't assign replicas to nonexi…
chia7712 opened a new pull request #10908:
URL: https://github.com/apache/kafka/pull/10908
The broker id starts with 1 (https://github.com/apache/kafka/blob/trunk/tests/kafkatest/services/kafka/kafka.py#L207) so `round_trip_fault_test.py` do assign replica to nonexistent broker.
The interesting story is that it fails on KRaft only. KRaft mode check the existent ids (https://github.com/apache/kafka/blob/trunk/metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java#L950). By contrast, zk mode have no such check and the `min.insync.replicas` is set with `1` so it can work on zk mode even though there is one replica is off-line.
### 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] chia7712 commented on pull request #10908: MINOR: fix round_trip_fault_test.py - don't assign replicas to nonexi…
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #10908:
URL: https://github.com/apache/kafka/pull/10908#issuecomment-865056585
> there is a behavior difference between the ZK-based KRaft controllers. Do we explicitly acknowledge and accept this difference?
that is a good point.
--
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] chia7712 commented on pull request #10908: MINOR: fix round_trip_fault_test.py - don't assign replicas to nonexi…
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #10908:
URL: https://github.com/apache/kafka/pull/10908#issuecomment-865056585
> there is a behavior difference between the ZK-based KRaft controllers. Do we explicitly acknowledge and accept this difference?
that is a good point.
--
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] ijuma commented on pull request #10908: MINOR: fix round_trip_fault_test.py - don't assign replicas to nonexi…
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10908:
URL: https://github.com/apache/kafka/pull/10908#issuecomment-865176496
> there is a behavior difference between the ZK-based KRaft controllers. Do we explicitly acknowledge and accept this difference?
I think we should, but happy to consider other opinions.
--
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] ijuma commented on pull request #10908: MINOR: fix round_trip_fault_test.py - don't assign replicas to nonexi…
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10908:
URL: https://github.com/apache/kafka/pull/10908#issuecomment-865176496
> there is a behavior difference between the ZK-based KRaft controllers. Do we explicitly acknowledge and accept this difference?
I think we should, but happy to consider other opinions.
--
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] rondagostino edited a comment on pull request #10908: MINOR: fix round_trip_fault_test.py - don't assign replicas to nonexi…
Posted by GitBox <gi...@apache.org>.
rondagostino edited a comment on pull request #10908:
URL: https://github.com/apache/kafka/pull/10908#issuecomment-865055596
Sorry to come to this party a bit late. It is likely that the code had a mistake and broker.id=0 should have been broker.id=1. But even with the mistake, I wonder if this test should have passed anyway given that the test with the ZK-based controller succeeded. In other words, the check at https://github.com/apache/kafka/blob/trunk/metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java#L950 means there is a behavior difference between the ZK-based and KRaft controllers. Do we explicitly acknowledge and accept this difference? cc @cmccabe
--
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] chia7712 merged pull request #10908: MINOR: fix round_trip_fault_test.py - don't assign replicas to nonexi…
Posted by GitBox <gi...@apache.org>.
chia7712 merged pull request #10908:
URL: https://github.com/apache/kafka/pull/10908
--
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] rondagostino commented on pull request #10908: MINOR: fix round_trip_fault_test.py - don't assign replicas to nonexi…
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #10908:
URL: https://github.com/apache/kafka/pull/10908#issuecomment-865055596
Sorry to come to this party a bit late. It is likely that the code had a mistake and broker.id=0 should have been broker.id=1. But even with the mistake, I wonder if this test should have passed anyway given that the test with the ZK-based controller succeeded. In other words, the check at https://github.com/apache/kafka/blob/trunk/metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java#L950 means there is a behavior difference between the ZK-based KRaft controllers. Do we explicitly acknowledge and accept this difference? cc @cmccabe
--
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] rondagostino commented on pull request #10908: MINOR: fix round_trip_fault_test.py - don't assign replicas to nonexi…
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #10908:
URL: https://github.com/apache/kafka/pull/10908#issuecomment-865055596
Sorry to come to this party a bit late. It is likely that the code had a mistake and broker.id=0 should have been broker.id=1. But even with the mistake, I wonder if this test should have passed anyway given that the test with the ZK-based controller succeeded. In other words, the check at https://github.com/apache/kafka/blob/trunk/metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java#L950 means there is a behavior difference between the ZK-based KRaft controllers. Do we explicitly acknowledge and accept this difference? cc @cmccabe
--
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] rondagostino edited a comment on pull request #10908: MINOR: fix round_trip_fault_test.py - don't assign replicas to nonexi…
Posted by GitBox <gi...@apache.org>.
rondagostino edited a comment on pull request #10908:
URL: https://github.com/apache/kafka/pull/10908#issuecomment-865055596
Sorry to come to this party a bit late. It is likely that the code had a mistake and broker.id=0 should have been broker.id=1. But even with the mistake, I wonder if this test should have passed anyway given that the test with the ZK-based controller succeeded. In other words, the check at https://github.com/apache/kafka/blob/trunk/metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java#L950 means there is a behavior difference between the ZK-based and KRaft controllers. Do we explicitly acknowledge and accept this difference? cc @cmccabe
--
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