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/03/03 20:56:34 UTC
[GitHub] [kafka] adamkotwasinski opened a new pull request #11843: MINOR: Correct Javadoc in FetchSessionHandler
adamkotwasinski opened a new pull request #11843:
URL: https://github.com/apache/kafka/pull/11843
While debugging some client issues I found out that some Javadocs in FetchSessionHandler got misleading / outdated.
### 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] jolshan commented on a change in pull request #11843: MINOR: Correct logging and Javadoc in FetchSessionHandler
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #11843:
URL: https://github.com/apache/kafka/pull/11843#discussion_r820992064
##########
File path: clients/src/main/java/org/apache/kafka/clients/FetchSessionHandler.java
##########
@@ -420,7 +420,7 @@ private String topicIdPartitionsToLogString(Collection<TopicIdPartition> partiti
* @param topicPartitions The topicPartitions from the FetchResponse.
* @param ids The topic IDs from the FetchResponse.
* @param version The version of the FetchResponse.
- * @return True if the full fetch response partitions are valid.
+ * @return null if the fetch response partitions are valid; human-readable problem description otherwise.
Review comment:
We can make this clearer if it helps. Maybe something like FullFetch? Not sure if there is a better option
--
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 pull request #11843: MINOR: Correct logging and Javadoc in FetchSessionHandler
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11843:
URL: https://github.com/apache/kafka/pull/11843#issuecomment-1061354885
Triggering jenkins build again to check the build results.
https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-11843/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] adamkotwasinski removed a comment on pull request #11843: MINOR: Correct Javadoc in FetchSessionHandler
Posted by GitBox <gi...@apache.org>.
adamkotwasinski removed a comment on pull request #11843:
URL: https://github.com/apache/kafka/pull/11843#issuecomment-1058647070
/retest
--
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] jolshan commented on a change in pull request #11843: MINOR: Correct Javadoc in FetchSessionHandler
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #11843:
URL: https://github.com/apache/kafka/pull/11843#discussion_r820912375
##########
File path: clients/src/main/java/org/apache/kafka/clients/FetchSessionHandler.java
##########
@@ -454,7 +454,7 @@ String verifyFullFetchResponsePartitions(Set<TopicPartition> topicPartitions, Se
* @param topicPartitions The topicPartitions from the FetchResponse.
* @param ids The topic IDs from the FetchResponse.
* @param version The version of the FetchResponse.
- * @return True if the incremental fetch response partitions are valid.
+ * @return null if the incremental fetch response partitions are valid; human-readable problem description otherwise.
Review comment:
These should be non-empty checks. Thanks for catching.
--
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 pull request #11843: MINOR: Correct logging and Javadoc in FetchSessionHandler
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11843:
URL: https://github.com/apache/kafka/pull/11843#issuecomment-1062687222
Failed tests are unrelated.
```
Build / ARM / kafka.network.SocketServerTest.testClientDisconnectionUpdatesRequestMetrics()
Build / PowerPC / org.apache.kafka.streams.integration.NamedTopologyIntegrationTest.shouldRemoveAndReplaceTopologicallyIncompatibleNamedTopology
Build / JDK 17 and Scala 2.13 / org.apache.kafka.connect.integration.BlockingConnectorTest.testBlockInSinkTaskStart
Build / JDK 17 and Scala 2.13 / org.apache.kafka.connect.integration.ConnectWorkerIntegrationTest.testBrokerCoordinator
Build / JDK 17 and Scala 2.13 / kafka.admin.DescribeConsumerGroupTest.testDescribeOffsetsOfExistingGroupWithNoMembers()
Build / JDK 17 and Scala 2.13 / kafka.admin.LeaderElectionCommandTest.[1] Type=Raft, Name=testElectionResultOutput, Security=PLAINTEXT
Build / JDK 17 and Scala 2.13 / kafka.server.DynamicBrokerReconfigurationTest.testThreadPoolResize()
Build / JDK 17 and Scala 2.13 / kafka.server.DynamicBrokerReconfigurationTest.testThreadPoolResize()
Build / JDK 11 and Scala 2.13 / kafka.server.DynamicBrokerReconfigurationTest.testThreadPoolResize()
Build / JDK 11 and Scala 2.13 / kafka.server.DynamicBrokerReconfigurationTest.testThreadPoolResize()
```
--
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] adamkotwasinski commented on a change in pull request #11843: MINOR: Correct Javadoc in FetchSessionHandler
Posted by GitBox <gi...@apache.org>.
adamkotwasinski commented on a change in pull request #11843:
URL: https://github.com/apache/kafka/pull/11843#discussion_r820979811
##########
File path: clients/src/main/java/org/apache/kafka/clients/FetchSessionHandler.java
##########
@@ -420,7 +420,7 @@ private String topicIdPartitionsToLogString(Collection<TopicIdPartition> partiti
* @param topicPartitions The topicPartitions from the FetchResponse.
* @param ids The topic IDs from the FetchResponse.
* @param version The version of the FetchResponse.
- * @return True if the full fetch response partitions are valid.
+ * @return null if the fetch response partitions are valid; human-readable problem description otherwise.
Review comment:
Ah! For the whole time I parsed it as `full (fetch response partitions)` and not `(full fetch) response partitions`, as the requests are named `Fetch` vs `IncrementalFetch`. Will add it back.
--
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] adamkotwasinski commented on a change in pull request #11843: MINOR: Correct logging and Javadoc in FetchSessionHandler
Posted by GitBox <gi...@apache.org>.
adamkotwasinski commented on a change in pull request #11843:
URL: https://github.com/apache/kafka/pull/11843#discussion_r820997952
##########
File path: clients/src/main/java/org/apache/kafka/clients/FetchSessionHandler.java
##########
@@ -420,7 +420,7 @@ private String topicIdPartitionsToLogString(Collection<TopicIdPartition> partiti
* @param topicPartitions The topicPartitions from the FetchResponse.
* @param ids The topic IDs from the FetchResponse.
* @param version The version of the FetchResponse.
- * @return True if the full fetch response partitions are valid.
+ * @return null if the fetch response partitions are valid; human-readable problem description otherwise.
Review comment:
I think we are fine - the implication comes from method signature `verifyFullFetchResponsePartitions` and its Javadoc `Verify that a full fetch response...`
--
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] dajac commented on pull request #11843: MINOR: Correct logging and Javadoc in FetchSessionHandler
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #11843:
URL: https://github.com/apache/kafka/pull/11843#issuecomment-1062644438
@showuon Can we merge this one?
--
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] adamkotwasinski commented on a change in pull request #11843: MINOR: Correct logging and Javadoc in FetchSessionHandler
Posted by GitBox <gi...@apache.org>.
adamkotwasinski commented on a change in pull request #11843:
URL: https://github.com/apache/kafka/pull/11843#discussion_r820984291
##########
File path: clients/src/main/java/org/apache/kafka/clients/FetchSessionHandler.java
##########
@@ -454,7 +454,7 @@ String verifyFullFetchResponsePartitions(Set<TopicPartition> topicPartitions, Se
* @param topicPartitions The topicPartitions from the FetchResponse.
* @param ids The topic IDs from the FetchResponse.
* @param version The version of the FetchResponse.
- * @return True if the incremental fetch response partitions are valid.
+ * @return null if the incremental fetch response partitions are valid; human-readable problem description otherwise.
Review comment:
Yeah, that should be safe, the built string is anyways always returned if either of them is non-empty. Amended.
--
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] adamkotwasinski commented on a change in pull request #11843: MINOR: Correct logging and Javadoc in FetchSessionHandler
Posted by GitBox <gi...@apache.org>.
adamkotwasinski commented on a change in pull request #11843:
URL: https://github.com/apache/kafka/pull/11843#discussion_r820984291
##########
File path: clients/src/main/java/org/apache/kafka/clients/FetchSessionHandler.java
##########
@@ -454,7 +454,7 @@ String verifyFullFetchResponsePartitions(Set<TopicPartition> topicPartitions, Se
* @param topicPartitions The topicPartitions from the FetchResponse.
* @param ids The topic IDs from the FetchResponse.
* @param version The version of the FetchResponse.
- * @return True if the incremental fetch response partitions are valid.
+ * @return null if the incremental fetch response partitions are valid; human-readable problem description otherwise.
Review comment:
Yeah, that should be safe, the built string is anyways always returned if either of them is non-empty.
--
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 merged pull request #11843: MINOR: Correct logging and Javadoc in FetchSessionHandler
Posted by GitBox <gi...@apache.org>.
showuon merged pull request #11843:
URL: https://github.com/apache/kafka/pull/11843
--
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 change in pull request #11843: MINOR: Correct Javadoc in FetchSessionHandler
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11843:
URL: https://github.com/apache/kafka/pull/11843#discussion_r819220063
##########
File path: clients/src/main/java/org/apache/kafka/clients/FetchSessionHandler.java
##########
@@ -454,7 +454,7 @@ String verifyFullFetchResponsePartitions(Set<TopicPartition> topicPartitions, Se
* @param topicPartitions The topicPartitions from the FetchResponse.
* @param ids The topic IDs from the FetchResponse.
* @param version The version of the FetchResponse.
- * @return True if the incremental fetch response partitions are valid.
+ * @return null if the incremental fetch response partitions are valid; human-readable problem description otherwise.
Review comment:
Thanks for the nice catch! I'm wondering if the L467 and L469 are wrong. Currently, we did:
```java
if (extra.isEmpty())
bld.append("extraPartitions=(").append(Utils.join(extra, ", ")).append("), ");
if (extraIds.isEmpty())
bld.append("extraIds=(").append(Utils.join(extraIds, ", ")).append("), ");
```
And that will only append **empty** extras and extraIds, which is not right. Do you think it should be non-empty check?
##########
File path: clients/src/main/java/org/apache/kafka/clients/FetchSessionHandler.java
##########
@@ -420,7 +420,7 @@ private String topicIdPartitionsToLogString(Collection<TopicIdPartition> partiti
* @param topicPartitions The topicPartitions from the FetchResponse.
* @param ids The topic IDs from the FetchResponse.
* @param version The version of the FetchResponse.
- * @return True if the full fetch response partitions are valid.
+ * @return null if the fetch response partitions are valid; human-readable problem description otherwise.
Review comment:
I would keep the `full` word as before. Any reason why we should remove it?
--
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] adamkotwasinski commented on pull request #11843: MINOR: Correct Javadoc in FetchSessionHandler
Posted by GitBox <gi...@apache.org>.
adamkotwasinski commented on pull request #11843:
URL: https://github.com/apache/kafka/pull/11843#issuecomment-1058647070
/retest
--
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] jolshan commented on a change in pull request #11843: MINOR: Correct Javadoc in FetchSessionHandler
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #11843:
URL: https://github.com/apache/kafka/pull/11843#discussion_r820912963
##########
File path: clients/src/main/java/org/apache/kafka/clients/FetchSessionHandler.java
##########
@@ -420,7 +420,7 @@ private String topicIdPartitionsToLogString(Collection<TopicIdPartition> partiti
* @param topicPartitions The topicPartitions from the FetchResponse.
* @param ids The topic IDs from the FetchResponse.
* @param version The version of the FetchResponse.
- * @return True if the full fetch response partitions are valid.
+ * @return null if the fetch response partitions are valid; human-readable problem description otherwise.
Review comment:
+1 to 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