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 2023/01/09 11:18:06 UTC
[GitHub] [kafka] mimaison opened a new pull request, #13094: MINOR: Various cleanups in client tests
mimaison opened a new pull request, #13094:
URL: https://github.com/apache/kafka/pull/13094
- Simplify assertions
- Remove redundant types
- Use lambdas instead of anonymous classes
- Remove unnecessary throws
### 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
[GitHub] [kafka] mimaison commented on a diff in pull request #13094: MINOR: Various cleanups in client tests
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #13094:
URL: https://github.com/apache/kafka/pull/13094#discussion_r1083436604
##########
clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java:
##########
@@ -331,17 +331,15 @@ public void testStressfulSituation() throws Exception {
1024 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * 1024, CompressionType.NONE, 0);
List<Thread> threads = new ArrayList<>();
for (int i = 0; i < numThreads; i++) {
- threads.add(new Thread() {
- public void run() {
- for (int i = 0; i < msgs; i++) {
- try {
- accum.append(topic, i % numParts, 0L, key, value, Record.EMPTY_HEADERS, null, maxBlockTimeMs, false, time.milliseconds(), cluster);
- } catch (Exception e) {
- e.printStackTrace();
- }
+ threads.add(new Thread(() -> {
+ for (int i1 = 0; i1 < msgs; i1++) {
Review Comment:
Good spot! I had not realized IntelliJ had automatically named it `i1` which is a bit strange. I renamed it to `j`.
--
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 #13094: MINOR: Various cleanups in client tests
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #13094:
URL: https://github.com/apache/kafka/pull/13094#discussion_r1065634957
##########
clients/src/test/java/org/apache/kafka/clients/FetchSessionHandlerTest.java:
##########
@@ -320,7 +315,7 @@ public void testDoubleBuild() {
try {
builder.build();
fail("Expected calling build twice to fail.");
- } catch (Throwable t) {
+ } catch (NullPointerException npe) {
Review Comment:
Thank you. Looks good to merge!
--
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 a diff in pull request #13094: MINOR: Various cleanups in client tests
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #13094:
URL: https://github.com/apache/kafka/pull/13094#discussion_r1066628366
##########
clients/src/main/java/org/apache/kafka/clients/FetchSessionHandler.java:
##########
@@ -266,7 +266,14 @@ public void add(TopicPartition topicPartition, PartitionData data) {
}
}
+ /**
+ * Build a FetchRequestData for the provided partitions
+ * @throws IllegalStateException if it has already been called
+ */
public FetchRequestData build() {
+ if (next == null) {
+ throw new IllegalStateException("build() has already been called.");
Review Comment:
This is not a test change?
--
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] clolov commented on a diff in pull request #13094: MINOR: Various cleanups in client tests
Posted by GitBox <gi...@apache.org>.
clolov commented on code in PR #13094:
URL: https://github.com/apache/kafka/pull/13094#discussion_r1067891312
##########
clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java:
##########
@@ -331,17 +331,15 @@ public void testStressfulSituation() throws Exception {
1024 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * 1024, CompressionType.NONE, 0);
List<Thread> threads = new ArrayList<>();
for (int i = 0; i < numThreads; i++) {
- threads.add(new Thread() {
- public void run() {
- for (int i = 0; i < msgs; i++) {
- try {
- accum.append(topic, i % numParts, 0L, key, value, Record.EMPTY_HEADERS, null, maxBlockTimeMs, false, time.milliseconds(), cluster);
- } catch (Exception e) {
- e.printStackTrace();
- }
+ threads.add(new Thread(() -> {
+ for (int i1 = 0; i1 < msgs; i1++) {
Review Comment:
Nit: Does this need to be `i1` - are we iterating through the same types of things as `i` is iterating through? Can we leave it as `i` similar to the previous version or change it to `j` (or something similar)?
--
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 #13094: MINOR: Various cleanups in client tests
Posted by GitBox <gi...@apache.org>.
mimaison commented on code in PR #13094:
URL: https://github.com/apache/kafka/pull/13094#discussion_r1066839046
##########
clients/src/main/java/org/apache/kafka/clients/FetchSessionHandler.java:
##########
@@ -266,7 +266,14 @@ public void add(TopicPartition topicPartition, PartitionData data) {
}
}
+ /**
+ * Build a FetchRequestData for the provided partitions
+ * @throws IllegalStateException if it has already been called
+ */
public FetchRequestData build() {
+ if (next == null) {
+ throw new IllegalStateException("build() has already been called.");
Review Comment:
I've reverted that change, I'll address this issue in another 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
[GitHub] [kafka] mimaison merged pull request #13094: MINOR: Various cleanups in client tests
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison merged PR #13094:
URL: https://github.com/apache/kafka/pull/13094
--
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 #13094: MINOR: Various cleanups in client tests
Posted by GitBox <gi...@apache.org>.
mimaison commented on code in PR #13094:
URL: https://github.com/apache/kafka/pull/13094#discussion_r1065495121
##########
clients/src/test/java/org/apache/kafka/clients/FetchSessionHandlerTest.java:
##########
@@ -320,7 +315,7 @@ public void testDoubleBuild() {
try {
builder.build();
fail("Expected calling build twice to fail.");
- } catch (Throwable t) {
+ } catch (NullPointerException npe) {
Review Comment:
You're right! I've pushed an update with a few changes to improve 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
[GitHub] [kafka] divijvaidya commented on a diff in pull request #13094: MINOR: Various cleanups in client tests
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #13094:
URL: https://github.com/apache/kafka/pull/13094#discussion_r1064594474
##########
clients/src/test/java/org/apache/kafka/clients/FetchSessionHandlerTest.java:
##########
@@ -320,7 +315,7 @@ public void testDoubleBuild() {
try {
builder.build();
fail("Expected calling build twice to fail.");
- } catch (Throwable t) {
+ } catch (NullPointerException npe) {
Review Comment:
NPE is a weird way of enforcing idempotency for build()! While you are in this code base could we add a javadoc to build() to clarify this expectation?
--
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] showuon commented on a diff in pull request #13094: MINOR: Various cleanups in client tests
Posted by GitBox <gi...@apache.org>.
showuon commented on code in PR #13094:
URL: https://github.com/apache/kafka/pull/13094#discussion_r1066625107
##########
clients/src/test/java/org/apache/kafka/common/security/oauthbearer/OAuthBearerSaslClientCallbackHandlerTest.java:
##########
@@ -32,7 +32,7 @@
import org.apache.kafka.common.security.oauthbearer.internals.OAuthBearerSaslClientCallbackHandler;
import org.junit.jupiter.api.Test;
-public class OAuthBearerSaslClienCallbackHandlerTest {
+public class OAuthBearerSaslClientCallbackHandlerTest {
Review Comment:
Aha, nice catch!
##########
clients/src/test/java/org/apache/kafka/common/utils/ExitTest.java:
##########
@@ -34,7 +34,7 @@ public void shouldHaltImmediately() {
});
try {
int statusCode = 0;
- String message = "mesaage";
+ String message = "message";
Review Comment:
Wow, how could you find it! Cool!
--
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