You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "leaf-soba (via GitHub)" <gi...@apache.org> on 2025/02/12 08:45:22 UTC

[PR] MINOR:use toList() instead of Collectors.toList() [kafka]

leaf-soba opened a new pull request, #18868:
URL: https://github.com/apache/kafka/pull/18868

   - replace `Stream.collect(Collectors.toList()) ` with `Stream.toList()`
   


-- 
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] MINOR:use toList() instead of Collectors.toList() [kafka]

Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #18868:
URL: https://github.com/apache/kafka/pull/18868#issuecomment-2658228762

   Note that these methods are not the same, `toList` results in an immutable list. I tried this before and it caused a few tests to fail, so we need to be careful when making this 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

Re: [PR] MINOR:use toList() instead of Collectors.toList() [kafka]

Posted by "leaf-soba (via GitHub)" <gi...@apache.org>.
leaf-soba closed pull request #18868: MINOR:use toList() instead of Collectors.toList()
URL: https://github.com/apache/kafka/pull/18868


-- 
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] MINOR:use toList() instead of Collectors.toList() [kafka]

Posted by "leaf-soba (via GitHub)" <gi...@apache.org>.
leaf-soba commented on PR #18868:
URL: https://github.com/apache/kafka/pull/18868#issuecomment-2658420820

   > Note that these methods are not the same, `toList` results in an immutable list. I tried this before and it caused a few tests to fail, so we need to be careful when making this change.
   > 
   > The hard part is validating the cases where the tests do _not_ fail (as there could be missing coverage).
   
   @ijuma Thanks for the note! I agree that this PR involves a large change with high risks and minimal benefits to the project. Given the potential issues, I think it would be better to close this PR and create a smaller one that focuses on other refactoring 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.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] MINOR:use toList() instead of Collectors.toList() [kafka]

Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on code in PR #18868:
URL: https://github.com/apache/kafka/pull/18868#discussion_r1952318577


##########
clients/src/main/java/org/apache/kafka/clients/ClusterConnectionStates.java:
##########
@@ -457,7 +457,7 @@ public boolean isConnectionSetupTimeout(String id, long now) {
     public List<String> nodesWithConnectionSetupTimeout(long now) {
         return connectingNodes.stream()
             .filter(id -> isConnectionSetupTimeout(id, now))
-            .collect(Collectors.toList());
+            .toList();

Review Comment:
   If I understand correctly, `Stream.toList()` was introduced in Java 16. The clients build at Java 11, while the broker builds at Java 17. As a result, I don't think you can use `Stream.toList()` in the clients project.



-- 
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] MINOR:use toList() instead of Collectors.toList() [kafka]

Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on code in PR #18868:
URL: https://github.com/apache/kafka/pull/18868#discussion_r1952566153


##########
clients/src/main/java/org/apache/kafka/clients/ClusterConnectionStates.java:
##########
@@ -457,7 +457,7 @@ public boolean isConnectionSetupTimeout(String id, long now) {
     public List<String> nodesWithConnectionSetupTimeout(long now) {
         return connectingNodes.stream()
             .filter(id -> isConnectionSetupTimeout(id, now))
-            .collect(Collectors.toList());
+            .toList();

Review Comment:
   Yes.
   We couldn't invoke `toList()` in modules that still use Java 11.
   Please refer to the following:
   https://github.com/apache/kafka/blob/c70b7c4b9e6fbd805d9a229b4ac13c1e10016d76/build.gradle#L52



-- 
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] MINOR:use toList() instead of Collectors.toList() [kafka]

Posted by "leaf-soba (via GitHub)" <gi...@apache.org>.
leaf-soba commented on PR #18868:
URL: https://github.com/apache/kafka/pull/18868#issuecomment-2658173219

   > Hi @leaf-soba It seems that you are struggling to pass the CI build. You could execute `./gradlew clean build -x test` locally; it will be more convenient than relying on CI. :)
   
   Thanks, I'll try to run it locally to pass the CI process.


-- 
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] MINOR:use toList() instead of Collectors.toList() [kafka]

Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on PR #18868:
URL: https://github.com/apache/kafka/pull/18868#issuecomment-2658167681

   Hi @leaf-soba 
   It seems that you are struggling to pass the CI build.
   You could execute `./gradlew clean build -x test` locally; it will be more convenient than relying on CI. :)


-- 
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] MINOR:use toList() instead of Collectors.toList() [kafka]

Posted by "leaf-soba (via GitHub)" <gi...@apache.org>.
leaf-soba commented on code in PR #18868:
URL: https://github.com/apache/kafka/pull/18868#discussion_r1953667432


##########
clients/src/main/java/org/apache/kafka/clients/ClusterConnectionStates.java:
##########
@@ -457,7 +457,7 @@ public boolean isConnectionSetupTimeout(String id, long now) {
     public List<String> nodesWithConnectionSetupTimeout(long now) {
         return connectingNodes.stream()
             .filter(id -> isConnectionSetupTimeout(id, now))
-            .collect(Collectors.toList());
+            .toList();

Review Comment:
   > If I understand correctly, `Stream.toList()` was introduced in Java 16. The clients build at Java 11, while the broker builds at Java 17. As a result, I don't think you can use `Stream.toList()` in the clients project.
   
   > Yes. We couldn't invoke `toList()` in modules that still use Java 11. Please refer to the following:
   > 
   > https://github.com/apache/kafka/blob/c70b7c4b9e6fbd805d9a229b4ac13c1e10016d76/build.gradle#L52
   
   Thanks for the clarification! Apologies for not checking the Java version of the modules more carefully. I have reverted the affected modules still using Java 11 to .collect(Collectors.toList());.



-- 
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