You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "justinrlee (via GitHub)" <gi...@apache.org> on 2024/11/12 08:51:00 UTC
[PR] (FIX): Support UpdateRaftVoterRequest in KafkaNetworkChannel [kafka]
justinrlee opened a new pull request, #17773:
URL: https://github.com/apache/kafka/pull/17773
*More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.*
*Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.*
### 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18001: Support UpdateRaftVoterRequest in KafkaNetworkChannel (WIP) [kafka]
Posted by "justinrlee (via GitHub)" <gi...@apache.org>.
justinrlee commented on PR #17773:
URL: https://github.com/apache/kafka/pull/17773#issuecomment-2472928978
Thanks, @ahuang98! I have updated the PR title and will look at adding tests today or tomorrow.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18001: Support UpdateRaftVoterRequest in KafkaNetworkChannel [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah merged PR #17773:
URL: https://github.com/apache/kafka/pull/17773
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (FIX): Support UpdateRaftVoterRequest in KafkaNetworkChannel [kafka]
Posted by "ahuang98 (via GitHub)" <gi...@apache.org>.
ahuang98 commented on PR #17773:
URL: https://github.com/apache/kafka/pull/17773#issuecomment-2471830563
Thanks @justinrlee, created https://issues.apache.org/jira/browse/KAFKA-18001 to track, can you prepend your PR title with `KAFKA-18001:`?
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18001: Support UpdateRaftVoterRequest in KafkaNetworkChannel (WIP) [kafka]
Posted by "justinrlee (via GitHub)" <gi...@apache.org>.
justinrlee commented on PR #17773:
URL: https://github.com/apache/kafka/pull/17773#issuecomment-2473818587
Okay, @ahuang98 , I've added tests similar to the existing tests in KafkaNetworkChannelTest; please let me know what else you need from me!
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18001: Support UpdateRaftVoterRequest in KafkaNetworkChannel [kafka]
Posted by "OmniaGM (via GitHub)" <gi...@apache.org>.
OmniaGM commented on PR #17773:
URL: https://github.com/apache/kafka/pull/17773#issuecomment-2478554914
LGTM, it would be nice if we can add some ducktape test for the KIP at some point. I don't think this suggestion should block this PR
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18001: Support UpdateRaftVoterRequest in KafkaNetworkChannel (WIP) [kafka]
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #17773:
URL: https://github.com/apache/kafka/pull/17773#discussion_r1841009664
##########
raft/src/main/java/org/apache/kafka/raft/KafkaNetworkChannel.java:
##########
@@ -187,6 +189,8 @@ static AbstractRequest.Builder<? extends AbstractRequest> buildRequest(ApiMessag
return new FetchRequest.SimpleBuilder((FetchRequestData) requestData);
if (requestData instanceof FetchSnapshotRequestData)
return new FetchSnapshotRequest.Builder((FetchSnapshotRequestData) requestData);
+ if (requestData instanceof UpdateRaftVoterRequestData)
Review Comment:
Do the controllers send `RemoveRaftVoterRequestData` and `AddRaftVoterRequestData` here? I don't think so?
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18001: Support UpdateRaftVoterRequest in KafkaNetworkChannel (WIP) [kafka]
Posted by "OmniaGM (via GitHub)" <gi...@apache.org>.
OmniaGM commented on code in PR #17773:
URL: https://github.com/apache/kafka/pull/17773#discussion_r1840517434
##########
raft/src/main/java/org/apache/kafka/raft/KafkaNetworkChannel.java:
##########
@@ -187,6 +189,8 @@ static AbstractRequest.Builder<? extends AbstractRequest> buildRequest(ApiMessag
return new FetchRequest.SimpleBuilder((FetchRequestData) requestData);
if (requestData instanceof FetchSnapshotRequestData)
return new FetchSnapshotRequest.Builder((FetchSnapshotRequestData) requestData);
+ if (requestData instanceof UpdateRaftVoterRequestData)
Review Comment:
I believe `RemoveRaftVoterRequestData` needs to be added as well.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18001: Support UpdateRaftVoterRequest in KafkaNetworkChannel [kafka]
Posted by "justinrlee (via GitHub)" <gi...@apache.org>.
justinrlee commented on PR #17773:
URL: https://github.com/apache/kafka/pull/17773#issuecomment-2475844254
@ahuang98
Added checkstyle suppression to the global suppression list; let me know if you need me to do something else!
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18001: Support UpdateRaftVoterRequest in KafkaNetworkChannel [kafka]
Posted by "ahuang98 (via GitHub)" <gi...@apache.org>.
ahuang98 commented on PR #17773:
URL: https://github.com/apache/kafka/pull/17773#issuecomment-2477683143
@justinrlee thanks for the fix! getting this into 3.9 would be a manual step - after this is merged into trunk, you can open up a PR targeting the `3.9` branch with your cherry-pick (and tag @cmccabe to merge it in)
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18001: Support UpdateRaftVoterRequest in KafkaNetworkChannel [kafka]
Posted by "justinrlee (via GitHub)" <gi...@apache.org>.
justinrlee commented on code in PR #17773:
URL: https://github.com/apache/kafka/pull/17773#discussion_r1841861864
##########
raft/src/main/java/org/apache/kafka/raft/KafkaNetworkChannel.java:
##########
@@ -187,6 +189,8 @@ static AbstractRequest.Builder<? extends AbstractRequest> buildRequest(ApiMessag
return new FetchRequest.SimpleBuilder((FetchRequestData) requestData);
if (requestData instanceof FetchSnapshotRequestData)
return new FetchSnapshotRequest.Builder((FetchSnapshotRequestData) requestData);
+ if (requestData instanceof UpdateRaftVoterRequestData)
Review Comment:
FWIW, I don't see any errors when I use `remove-controller` so it doesn't look like we need to support this.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (FIX): Support UpdateRaftVoterRequest in KafkaNetworkChannel [kafka]
Posted by "ahuang98 (via GitHub)" <gi...@apache.org>.
ahuang98 commented on PR #17773:
URL: https://github.com/apache/kafka/pull/17773#issuecomment-2471284026
cc @jsancio
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18001: Support UpdateRaftVoterRequest in KafkaNetworkChannel (WIP) [kafka]
Posted by "ahuang98 (via GitHub)" <gi...@apache.org>.
ahuang98 commented on code in PR #17773:
URL: https://github.com/apache/kafka/pull/17773#discussion_r1841016340
##########
raft/src/main/java/org/apache/kafka/raft/KafkaNetworkChannel.java:
##########
@@ -187,6 +189,8 @@ static AbstractRequest.Builder<? extends AbstractRequest> buildRequest(ApiMessag
return new FetchRequest.SimpleBuilder((FetchRequestData) requestData);
if (requestData instanceof FetchSnapshotRequestData)
return new FetchSnapshotRequest.Builder((FetchSnapshotRequestData) requestData);
+ if (requestData instanceof UpdateRaftVoterRequestData)
Review Comment:
that's what I thought as well, I don't think we need to add either here
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18001: Support UpdateRaftVoterRequest in KafkaNetworkChannel (WIP) [kafka]
Posted by "justinrlee (via GitHub)" <gi...@apache.org>.
justinrlee commented on PR #17773:
URL: https://github.com/apache/kafka/pull/17773#issuecomment-2475213784
Looks like all the tests are failing due to my new tests bumping us over the threshold (25)for the `ClassDataAbstractionCoupling` checkstyle:
```
Error: eckstyle] [ERROR] /home/runner/work/kafka/kafka/raft/src/test/java/org/apache/kafka/raft/KafkaNetworkChannelTest.java:68:1: Class Data Abstraction Coupling is 26 (max allowed is 25) classes [AssertionError, BeginQuorumEpochResponse, BeginQuorumEpochResponseData, EndQuorumEpochResponse, EndQuorumEpochResponseData, FetchRequestData.ReplicaState, FetchResponse, FetchResponseData, FetchSnapshotResponse, FetchSnapshotResponseData, KafkaNetworkChannel, MockClient, MockTime, Node, OffsetAndEpoch, RaftRequest.Outbound, StubMetadataUpdater, SupportedVersionRange, Thread, TopicPartition, UpdateRaftVoterResponse, UpdateRaftVoterResponseData, VoteResponse, VoteResponseData, VoteResponseData.PartitionData, VoteResponseData.TopicData]. [ClassDataAbstractionCoupling]
```
Two questions:
* Is it acceptable to just suppress this warning? I assume so, because I see a ton of suppressions for similar test cases across the project
* Is it preferable to suppress in the suppress in the global [checkstyle suppressions file](https://github.com/apache/kafka/blob/trunk/checkstyle/suppressions.xml), or is it preferable to do an inline suppression (I see a [handful](https://github.com/apache/kafka/blob/4fcb8b247ec28b9f6e64f8f663e34f5becf0f25f/tools/src/main/java/org/apache/kafka/tools/reassign/ReassignPartitionsCommand.java#L77) of [these](https://github.com/apache/kafka/blob/4fcb8b247ec28b9f6e64f8f663e34f5becf0f25f/connect/runtime/src/test/java/org/apache/kafka/connect/util/TopicAdminTest.java#L86) [across](https://github.com/apache/kafka/blob/4fcb8b247ec28b9f6e64f8f663e34f5becf0f25f/group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupCoordinatorRecordSerdeTest.java#L67) [the](https://github.com/apache/kafka/blob/4fcb8b247ec28b9f6e64f8f663e34f5becf0f25f/storage/src/test/java/org/apache/kafka/tiered/storage/TieredStorageTestBuilder.java#L70) [project](https://github.com/apache/kafka/blob/4fcb8b2
47ec28b9f6e64f8f663e34f5becf0f25f/clients/src/test/java/org/apache/kafka/clients/consumer/internals/events/ApplicationEventProcessorTest.java#L77))
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18001: Support UpdateRaftVoterRequest in KafkaNetworkChannel [kafka]
Posted by "ahuang98 (via GitHub)" <gi...@apache.org>.
ahuang98 commented on PR #17773:
URL: https://github.com/apache/kafka/pull/17773#issuecomment-2479537038
@OmniaGM we do have https://github.com/apache/kafka/blob/trunk/tests/kafkatest/tests/core/quorum_reconfiguration_test.py but you're right that it doesn't cover this case due to the quorum being brought up with kraft.version=1 already. The test Justin added is pretty straightforward in confirming UpdateRaftVoterRequests can make it through
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org