You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "m1a2st (via GitHub)" <gi...@apache.org> on 2025/10/07 13:33:37 UTC
[PR] MINOR: Followup KAFKA-18193 for null check and error message [kafka]
m1a2st opened a new pull request, #20650:
URL: https://github.com/apache/kafka/pull/20650
See the comments, and followup https://github.com/apache/kafka/pull/19955#pullrequestreview-3310028108
--
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: Followup KAFKA-18193 for null check and error message [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #20650:
URL: https://github.com/apache/kafka/pull/20650
--
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: Followup KAFKA-18193 for null check and error message [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20650:
URL: https://github.com/apache/kafka/pull/20650#discussion_r2421328079
##########
streams/src/main/java/org/apache/kafka/streams/KafkaStreams.java:
##########
@@ -1635,8 +1635,9 @@ public synchronized boolean close(final CloseOptions options) throws IllegalArgu
public synchronized boolean close(final org.apache.kafka.streams.CloseOptions options) throws IllegalArgumentException {
Objects.requireNonNull(options, "options cannot be null");
final CloseOptionsInternal optionsInternal = new CloseOptionsInternal(options);
- final String msgPrefix = prepareMillisCheckFailMsgPrefix(optionsInternal.timeout(), "timeout");
- final long timeoutMs = validateMillisecondDuration(optionsInternal.timeout().get(), msgPrefix);
+ final Duration timeout = optionsInternal.timeout().orElseGet(() -> Duration.ofMillis(Long.MAX_VALUE));
Review Comment:
could you please add test for empty timeout?
--
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: Followup KAFKA-18193 for null check and error message [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20650:
URL: https://github.com/apache/kafka/pull/20650#discussion_r2410776459
##########
streams/src/main/java/org/apache/kafka/streams/CloseOptions.java:
##########
@@ -81,7 +81,11 @@ public static CloseOptions groupMembershipOperation(final GroupMembershipOperati
* @return this {@code CloseOptions} instance.
*/
public CloseOptions withTimeout(final Duration timeout) {
- this.timeout = Optional.ofNullable(timeout);
+ if (timeout == null) {
+ this.timeout = Optional.of(Duration.ofMillis(Long.MAX_VALUE));
Review Comment:
It looks like `timeout` is never `empty`. However, I think keeping the `empty` state is useful because it shows the user did not explicitly set a `timeout`. Maybe we can use `orElse` to address the comments
```java
var timeout = optionsInternal.timeout().orElse(Duration.ofMillis(Long.MAX_VALUE));
final String msgPrefix = prepareMillisCheckFailMsgPrefix(timeout, "timeout");
final long timeoutMs = validateMillisecondDuration(timeout, msgPrefix);
```
--
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: Followup KAFKA-18193 for null check and error message [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20650:
URL: https://github.com/apache/kafka/pull/20650#discussion_r2414632130
##########
streams/src/main/java/org/apache/kafka/streams/KafkaStreams.java:
##########
@@ -1635,8 +1635,9 @@ public synchronized boolean close(final CloseOptions options) throws IllegalArgu
public synchronized boolean close(final org.apache.kafka.streams.CloseOptions options) throws IllegalArgumentException {
Objects.requireNonNull(options, "options cannot be null");
final CloseOptionsInternal optionsInternal = new CloseOptionsInternal(options);
- final String msgPrefix = prepareMillisCheckFailMsgPrefix(optionsInternal.timeout(), "timeout");
- final long timeoutMs = validateMillisecondDuration(optionsInternal.timeout().get(), msgPrefix);
+ final Duration timeout = optionsInternal.timeout().orElse(Duration.ofMillis(Long.MAX_VALUE));
Review Comment:
Perhaps using `orElseGet` is more better, which can avoid creating `Duration`
--
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: Followup KAFKA-18193 for null check and error message [kafka]
Posted by "m1a2st (via GitHub)" <gi...@apache.org>.
m1a2st commented on code in PR #20650:
URL: https://github.com/apache/kafka/pull/20650#discussion_r2410802803
##########
streams/src/main/java/org/apache/kafka/streams/CloseOptions.java:
##########
@@ -81,7 +81,11 @@ public static CloseOptions groupMembershipOperation(final GroupMembershipOperati
* @return this {@code CloseOptions} instance.
*/
public CloseOptions withTimeout(final Duration timeout) {
- this.timeout = Optional.ofNullable(timeout);
+ if (timeout == null) {
+ this.timeout = Optional.of(Duration.ofMillis(Long.MAX_VALUE));
Review Comment:
Thanks, addressed 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