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 2022/09/27 18:08:51 UTC
[GitHub] [kafka] see-quick opened a new pull request, #12692: MINOR: Add test cases to the Raft module
see-quick opened a new pull request, #12692:
URL: https://github.com/apache/kafka/pull/12692
This PR adds a few test cases to the Raft module (i.e., VotedStateTest, ValidOffsetAndEpochTest, UnattachedStateTest, ResignedStateTest). I have also removed an un-used method (i.e., `overrideElectionTimeout`) from VotedState class.
Signed-off-by: morsak <ma...@gmail.com>
--
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
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12692: MINOR: Add test cases to the Raft module
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12692:
URL: https://github.com/apache/kafka/pull/12692#discussion_r982359239
##########
raft/src/main/java/org/apache/kafka/raft/VotedState.java:
##########
@@ -92,11 +92,6 @@ public boolean hasElectionTimeoutExpired(long currentTimeMs) {
return electionTimer.isExpired();
}
- public void overrideElectionTimeout(long currentTimeMs, long timeoutMs) {
Review Comment:
ok, I stand corrected. Apologies for the confusion here @see-quick. (note to myself) Here is the guidance that we can follow for what requires a KIP for future: https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals#KafkaImprovementProposals-Whatisconsidereda%22majorchange%22thatneedsaKIP?
--
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
[GitHub] [kafka] see-quick commented on a diff in pull request #12692: MINOR: Add test cases to the Raft module
Posted by GitBox <gi...@apache.org>.
see-quick commented on code in PR #12692:
URL: https://github.com/apache/kafka/pull/12692#discussion_r983614070
##########
raft/src/main/java/org/apache/kafka/raft/VotedState.java:
##########
@@ -92,11 +92,6 @@ public boolean hasElectionTimeoutExpired(long currentTimeMs) {
return electionTimer.isExpired();
}
- public void overrideElectionTimeout(long currentTimeMs, long timeoutMs) {
Review Comment:
I have got that commit in my local repo, so as you propose we need to ask a committer to re-run the tests :)
--
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
[GitHub] [kafka] see-quick commented on a diff in pull request #12692: MINOR: Add test cases to the Raft module
Posted by GitBox <gi...@apache.org>.
see-quick commented on code in PR #12692:
URL: https://github.com/apache/kafka/pull/12692#discussion_r982830205
##########
raft/src/main/java/org/apache/kafka/raft/VotedState.java:
##########
@@ -92,11 +92,6 @@ public boolean hasElectionTimeoutExpired(long currentTimeMs) {
return electionTimer.isExpired();
}
- public void overrideElectionTimeout(long currentTimeMs, long timeoutMs) {
Review Comment:
So basically we can merge it or? :) ^^ @divijvaidya ?
--
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
[GitHub] [kafka] see-quick commented on a diff in pull request #12692: MINOR: Add test cases to the Raft module
Posted by GitBox <gi...@apache.org>.
see-quick commented on code in PR #12692:
URL: https://github.com/apache/kafka/pull/12692#discussion_r982190906
##########
raft/src/main/java/org/apache/kafka/raft/VotedState.java:
##########
@@ -92,11 +92,6 @@ public boolean hasElectionTimeoutExpired(long currentTimeMs) {
return electionTimer.isExpired();
}
- public void overrideElectionTimeout(long currentTimeMs, long timeoutMs) {
Review Comment:
I will revert this method deletion and let only these test cases. Sounds good? I wonder if adding a test case for such an (un-used) method would be beneficial...
--
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
[GitHub] [kafka] mimaison merged pull request #12692: MINOR: Add test cases to the Raft module
Posted by GitBox <gi...@apache.org>.
mimaison merged PR #12692:
URL: https://github.com/apache/kafka/pull/12692
--
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
[GitHub] [kafka] see-quick commented on a diff in pull request #12692: MINOR: Add test cases to the Raft module
Posted by GitBox <gi...@apache.org>.
see-quick commented on code in PR #12692:
URL: https://github.com/apache/kafka/pull/12692#discussion_r982443619
##########
raft/src/main/java/org/apache/kafka/raft/VotedState.java:
##########
@@ -92,11 +92,6 @@ public boolean hasElectionTimeoutExpired(long currentTimeMs) {
return electionTimer.isExpired();
}
- public void overrideElectionTimeout(long currentTimeMs, long timeoutMs) {
Review Comment:
> ok, I stand corrected. Apologies for the confusion here @see-quick. (note to myself) Here is the guidance that we can follow for what requires a KIP for future: https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals#KafkaImprovementProposals-Whatisconsidereda%22majorchange%22thatneedsaKIP?
Thanks for sharing that link @divijvaidya <3
--
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
[GitHub] [kafka] ijuma commented on pull request #12692: MINOR: Add test cases to the Raft module
Posted by GitBox <gi...@apache.org>.
ijuma commented on PR #12692:
URL: https://github.com/apache/kafka/pull/12692#issuecomment-1264525103
Thanks for the PR. I don't understand the goal of the `toString` tests since they basically check that the toString doesn't have `@`?
--
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
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12692: MINOR: Add test cases to the Raft module
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12692:
URL: https://github.com/apache/kafka/pull/12692#discussion_r982147666
##########
raft/src/main/java/org/apache/kafka/raft/VotedState.java:
##########
@@ -92,11 +92,6 @@ public boolean hasElectionTimeoutExpired(long currentTimeMs) {
return electionTimer.isExpired();
}
- public void overrideElectionTimeout(long currentTimeMs, long timeoutMs) {
Review Comment:
I understand that it is an unused method but removing or adding public methods requires a KIP in the Kafka community.
--
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
[GitHub] [kafka] mimaison commented on a diff in pull request #12692: MINOR: Add test cases to the Raft module
Posted by GitBox <gi...@apache.org>.
mimaison commented on code in PR #12692:
URL: https://github.com/apache/kafka/pull/12692#discussion_r982348035
##########
raft/src/main/java/org/apache/kafka/raft/VotedState.java:
##########
@@ -92,11 +92,6 @@ public boolean hasElectionTimeoutExpired(long currentTimeMs) {
return electionTimer.isExpired();
}
- public void overrideElectionTimeout(long currentTimeMs, long timeoutMs) {
Review Comment:
`VotedState` is not part of the public API so we can change this public method without a KIP.
--
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
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12692: MINOR: Add test cases to the Raft module
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12692:
URL: https://github.com/apache/kafka/pull/12692#discussion_r983414080
##########
raft/src/main/java/org/apache/kafka/raft/VotedState.java:
##########
@@ -92,11 +92,6 @@ public boolean hasElectionTimeoutExpired(long currentTimeMs) {
return electionTimer.isExpired();
}
- public void overrideElectionTimeout(long currentTimeMs, long timeoutMs) {
Review Comment:
Yes code looks good from my side 👍
We need to re-run the tests since the build had a bunch of failures due to a lingering thread. We expect this commit https://github.com/apache/kafka/commit/09011da76d450f9a7dd11e159741f8e69cca115b to mitigate the lingering thread problem and unblock the build. Please check if you have this commit in your repo. If not, I would suggest to rebase this test with `trunk` and run again. If you already have this commit, then we need to ask a committer to re-run the tests.
--
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
[GitHub] [kafka] mimaison commented on a diff in pull request #12692: MINOR: Add test cases to the Raft module
Posted by GitBox <gi...@apache.org>.
mimaison commented on code in PR #12692:
URL: https://github.com/apache/kafka/pull/12692#discussion_r982449921
##########
raft/src/main/java/org/apache/kafka/raft/VotedState.java:
##########
@@ -92,11 +92,6 @@ public boolean hasElectionTimeoutExpired(long currentTimeMs) {
return electionTimer.isExpired();
}
- public void overrideElectionTimeout(long currentTimeMs, long timeoutMs) {
Review Comment:
You can find the list of public classes/interfaces in the javadocs: https://kafka.apache.org/32/javadoc/allclasses-index.html
--
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
[GitHub] [kafka] see-quick commented on a diff in pull request #12692: MINOR: Add test cases to the Raft module
Posted by GitBox <gi...@apache.org>.
see-quick commented on code in PR #12692:
URL: https://github.com/apache/kafka/pull/12692#discussion_r982473889
##########
raft/src/main/java/org/apache/kafka/raft/VotedState.java:
##########
@@ -92,11 +92,6 @@ public boolean hasElectionTimeoutExpired(long currentTimeMs) {
return electionTimer.isExpired();
}
- public void overrideElectionTimeout(long currentTimeMs, long timeoutMs) {
Review Comment:
Thanks noted.
--
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
[GitHub] [kafka] see-quick commented on pull request #12692: MINOR: Add test cases to the Raft module
Posted by GitBox <gi...@apache.org>.
see-quick commented on PR #12692:
URL: https://github.com/apache/kafka/pull/12692#issuecomment-1264624416
> Thanks for the PR. I don't understand the goal of the `toString` tests since they basically check that the toString doesn't have `@`?
The main purpose of such test cases is to check that it does print something useful. Meaning not printing object reference from the inherited superclass (i.e., `java.lang.Object@7fbe847c`). If you do not see any value in such test cases I can remove 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org