Re: [PR] MINOR: simplify ensure topic exists condition [kafka]
FrankYang0529 commented on code in PR #15458:
URL: https://github.com/apache/kafka/pull/15458#discussion_r1510195954
##
tools/src/main/java/org/apache/kafka/tools/TopicCommand.java:
##
@@ -210,7 +210,7 @@ private static Integer
getReplicationFactor(TopicPartitionInfo tpi, PartitionRea
*/
private static void ensureTopicExists(List foundTopics, String
requestedTopic, Boolean requireTopicExists) {
// If no topic name was mentioned, do not need to throw exception.
-if (!(requestedTopic.isEmpty() ||
!Optional.ofNullable(requestedTopic).isPresent()) && requireTopicExists &&
foundTopics.isEmpty()) {
+if (requestedTopic != null && !requestedTopic.isEmpty() &&
requireTopicExists && foundTopics.isEmpty()) {
Review Comment:
Updated it. Thanks.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16246: Cleanups in ConsoleConsumer [kafka]
wernerdv commented on PR #15457: URL: https://github.com/apache/kafka/pull/15457#issuecomment-1975051194 Hello, @mimaison @OmniaGM Please, take a look. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: simplify ensure topic exists condition [kafka]
chia7712 commented on code in PR #15458:
URL: https://github.com/apache/kafka/pull/15458#discussion_r1510162458
##
tools/src/main/java/org/apache/kafka/tools/TopicCommand.java:
##
@@ -210,7 +210,7 @@ private static Integer
getReplicationFactor(TopicPartitionInfo tpi, PartitionRea
*/
private static void ensureTopicExists(List foundTopics, String
requestedTopic, Boolean requireTopicExists) {
// If no topic name was mentioned, do not need to throw exception.
-if (!(requestedTopic.isEmpty() ||
!Optional.ofNullable(requestedTopic).isPresent()) && requireTopicExists &&
foundTopics.isEmpty()) {
+if (requestedTopic != null && !requestedTopic.isEmpty() &&
requireTopicExists && foundTopics.isEmpty()) {
Review Comment:
that makes sense to me. Could you change the type from `String` to
`Optional` to handle the null?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: simplify ensure topic exists condition [kafka]
FrankYang0529 commented on code in PR #15458:
URL: https://github.com/apache/kafka/pull/15458#discussion_r1510158945
##
tools/src/main/java/org/apache/kafka/tools/TopicCommand.java:
##
@@ -210,7 +210,7 @@ private static Integer
getReplicationFactor(TopicPartitionInfo tpi, PartitionRea
*/
private static void ensureTopicExists(List foundTopics, String
requestedTopic, Boolean requireTopicExists) {
// If no topic name was mentioned, do not need to throw exception.
-if (!(requestedTopic.isEmpty() ||
!Optional.ofNullable(requestedTopic).isPresent()) && requireTopicExists &&
foundTopics.isEmpty()) {
+if (requestedTopic != null && !requestedTopic.isEmpty() &&
requireTopicExists && foundTopics.isEmpty()) {
Review Comment:
Yeah, I know there is no null input case for `requestedTopic`. However, do
we want to check `requestedTopic != null` to prevent null case in the future?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[PR] MINOR: fix link for ListTransactionsOptions#filterOnDuration [kafka]
FrankYang0529 opened a new pull request, #15459: URL: https://github.com/apache/kafka/pull/15459 *More detailed description of your change, if necessary. The PR title and PR message become the squashed commit message, so use a separate comment to ping reviewers.* *Summary of testing strategy (including rationale) for the feature or bug fix. Unit and/or integration tests are expected for any behaviour change and system tests should be considered for larger changes.* ### 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[jira] [Assigned] (KAFKA-12187) replace assertTrue(obj instanceof X) by assertInstanceOf when we update to JUnit 5.8
[
https://issues.apache.org/jira/browse/KAFKA-12187?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Kuan-Po Tseng reassigned KAFKA-12187:
-
Assignee: Kuan-Po Tseng (was: Chia-Ping Tsai)
> replace assertTrue(obj instanceof X) by assertInstanceOf when we update to
> JUnit 5.8
>
>
> Key: KAFKA-12187
> URL: https://issues.apache.org/jira/browse/KAFKA-12187
> Project: Kafka
> Issue Type: Sub-task
>Reporter: Chia-Ping Tsai
>Assignee: Kuan-Po Tseng
>Priority: Minor
>
> see [https://github.com/apache/kafka/pull/9874#discussion_r556547909]
>
> {quote}Yeah, for existing code improvements (versus code introduced by this
> change), let's do it via a different PR. For this particular issue, we can
> probably wait for JUnit 5.8 and use:
> {quote}
> * New assertInstanceOf methods as a replacement for assertTrue(obj instanceof
> X) which provide better error messages comparable to those of assertThrows.
> related PR: https://github.com/junit-team/junit5/pull/2499
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
Re: [PR] MINOR: simplify ensure topic exists condition [kafka]
chia7712 commented on code in PR #15458:
URL: https://github.com/apache/kafka/pull/15458#discussion_r1510157302
##
tools/src/main/java/org/apache/kafka/tools/TopicCommand.java:
##
@@ -210,7 +210,7 @@ private static Integer
getReplicationFactor(TopicPartitionInfo tpi, PartitionRea
*/
private static void ensureTopicExists(List foundTopics, String
requestedTopic, Boolean requireTopicExists) {
// If no topic name was mentioned, do not need to throw exception.
-if (!(requestedTopic.isEmpty() ||
!Optional.ofNullable(requestedTopic).isPresent()) && requireTopicExists &&
foundTopics.isEmpty()) {
+if (requestedTopic != null && !requestedTopic.isEmpty() &&
requireTopicExists && foundTopics.isEmpty()) {
Review Comment:
it never pass null `requestedTopic`, and it results in
`!Optional.ofNullable(requestedTopic).isPresent()` being unnecessary condition
- it is always `false`
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[jira] [Resolved] (KAFKA-12218) Upgrade Junit to 5.8+
[ https://issues.apache.org/jira/browse/KAFKA-12218?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Chia-Ping Tsai resolved KAFKA-12218. Resolution: Duplicate this was resolved by https://github.com/apache/kafka/pull/15404 > Upgrade Junit to 5.8+ > - > > Key: KAFKA-12218 > URL: https://issues.apache.org/jira/browse/KAFKA-12218 > Project: Kafka > Issue Type: Sub-task >Reporter: Chia-Ping Tsai >Priority: Major > -- This message was sent by Atlassian Jira (v8.20.10#820010)
[PR] MINOR: simplify ensure topic exists condition [kafka]
FrankYang0529 opened a new pull request, #15458: URL: https://github.com/apache/kafka/pull/15458 Originally, `requestedTopic.isEmpty()` is before `!Optional.ofNullable(requestedTopic).isPresent()`. If `requestedTopic` is null, we get error from `requestedTopic.isEmpty()`. Simplify `!Optional.ofNullable(requestedTopic).isPresent()` as `requestedTopic != null` ans put it before `requestedTopic.isEmpty()`. ### 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Upgrade jqwik to version 1.8.2 [kafka]
chia7712 commented on PR #14365: URL: https://github.com/apache/kafka/pull/14365#issuecomment-1974982326 ``` [2024-03-02T21:51:02.002Z] kafka.api.PlaintextAdminIntegrationTest.testElectPreferredLeaders(String)[2] failed, log available in /home/jenkins/workspace/Kafka_kafka-pr_PR-14365/core/build/reports/testOutput/kafka.api.PlaintextAdminIntegrationTest.testElectPreferredLeaders(String)[2].test.stdout [2024-03-02T21:51:02.002Z] [2024-03-02T21:51:02.002Z] Gradle Test Run :core:test > Gradle Test Executor 94 > PlaintextAdminIntegrationTest > testElectPreferredLeaders(String) > testElectPreferredLeaders(String).quorum=kraft FAILED [2024-03-02T21:51:02.002Z] org.opentest4j.AssertionFailedError: Timed out waiting for leader to become Some(0). Last metadata lookup returned leader = Some(1) ==> expected: but was: [2024-03-02T21:51:02.002Z] at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151) [2024-03-02T21:51:02.002Z] at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132) [2024-03-02T21:51:02.002Z] at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63) [2024-03-02T21:51:02.002Z] at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36) [2024-03-02T21:51:02.002Z] at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:214) [2024-03-02T21:51:02.002Z] at kafka.utils.TestUtils$.waitForLeaderToBecome(TestUtils.scala:2116) [2024-03-02T21:51:02.002Z] at kafka.utils.TestUtils$.assertLeader(TestUtils.scala:2079) [2024-03-02T21:51:02.002Z] at kafka.api.PlaintextAdminIntegrationTest.changePreferredLeader$1(PlaintextAdminIntegrationTest.scala:1439) [2024-03-02T21:51:02.002Z] at kafka.api.PlaintextAdminIntegrationTest.testElectPreferredLeaders(PlaintextAdminIntegrationTest.scala:1501) ``` https://issues.apache.org/jira/browse/KAFKA-8458 ``` [2024-03-03T00:18:55.732Z] kafka.server.ReplicaManagerTest.testRemoteFetchExpiresPerSecMetric() failed, log available in /home/jenkins/workspace/Kafka_kafka-pr_PR-14365/core/build/reports/testOutput/kafka.server.ReplicaManagerTest.testRemoteFetchExpiresPerSecMetric().test.stdout [2024-03-03T00:18:55.732Z] [2024-03-03T00:18:55.732Z] Gradle Test Run :core:test > Gradle Test Executor 95 > ReplicaManagerTest > testRemoteFetchExpiresPerSecMetric() FAILED [2024-03-03T00:18:55.732Z] org.opentest4j.AssertionFailedError: The ExpiresPerSec value is not incremented. Current value is: 0 [2024-03-03T00:18:55.732Z] at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:38) [2024-03-03T00:18:55.732Z] at org.junit.jupiter.api.Assertions.fail(Assertions.java:138) [2024-03-03T00:18:55.732Z] at kafka.server.ReplicaManagerTest.testRemoteFetchExpiresPerSecMetric(ReplicaManagerTest.scala:4174) ``` https://issues.apache.org/jira/browse/KAFKA-13530 those failed tests are flaky since there are jira already. Also, they pass on my local. so +1 to this PR and I will merge it tomorrow if there is no objection -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[jira] [Updated] (KAFKA-16246) Cleanups in ConsoleConsumer
[ https://issues.apache.org/jira/browse/KAFKA-16246?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Dmitry Werner updated KAFKA-16246: -- Fix Version/s: 3.8.0 > Cleanups in ConsoleConsumer > --- > > Key: KAFKA-16246 > URL: https://issues.apache.org/jira/browse/KAFKA-16246 > Project: Kafka > Issue Type: Improvement > Components: tools >Reporter: Mickael Maison >Assignee: Dmitry Werner >Priority: Minor > Fix For: 3.8.0 > > > When rewriting ConsoleConsumer in Java, in order to keep the conversion and > review process simple we mimicked the logic flow and types used in the Scala > implementation. > Once the rewrite is merged, we should refactor some of the logic to make it > more Java-like. This include removing Optional where it makes sense and > moving all the argument checking logic into ConsoleConsumerOptions. > See https://github.com/apache/kafka/pull/15274 for pointers. > -- This message was sent by Atlassian Jira (v8.20.10#820010)
[PR] KAFKA-16246: Cleanups in ConsoleConsumer [kafka]
wernerdv opened a new pull request, #15457: URL: https://github.com/apache/kafka/pull/15457 Removed Optional where it makes sense and removed the argument checking logic that duplicates existing in ConsoleConsumerOptions#checkRequiredArgs(). ### 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Upgrade jqwik to version 1.8.2 [kafka]
bmscomp commented on PR #14365: URL: https://github.com/apache/kafka/pull/14365#issuecomment-1974904749 @chia7712 Jqwik version is updated to 1.8.3 and conflicts are resolved -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[jira] [Assigned] (KAFKA-16246) Cleanups in ConsoleConsumer
[ https://issues.apache.org/jira/browse/KAFKA-16246?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Dmitry Werner reassigned KAFKA-16246: - Assignee: Dmitry Werner > Cleanups in ConsoleConsumer > --- > > Key: KAFKA-16246 > URL: https://issues.apache.org/jira/browse/KAFKA-16246 > Project: Kafka > Issue Type: Improvement > Components: tools >Reporter: Mickael Maison >Assignee: Dmitry Werner >Priority: Minor > > When rewriting ConsoleConsumer in Java, in order to keep the conversion and > review process simple we mimicked the logic flow and types used in the Scala > implementation. > Once the rewrite is merged, we should refactor some of the logic to make it > more Java-like. This include removing Optional where it makes sense and > moving all the argument checking logic into ConsoleConsumerOptions. > See https://github.com/apache/kafka/pull/15274 for pointers. > -- This message was sent by Atlassian Jira (v8.20.10#820010)
Re: [PR] MINOR: Upgrade jqwik to version 1.8.2 [kafka]
chia7712 commented on PR #14365: URL: https://github.com/apache/kafka/pull/14365#issuecomment-1974864320 the last version of `jqwik` is 1.8.3. Maybe we should rebase the code with 1.8.3, and then merge it if there is no related failure. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15154: proper StampedLock synchronization added [kafka]
mstepan closed pull request #14837: KAFKA-15154: proper StampedLock synchronization added URL: https://github.com/apache/kafka/pull/14837 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[jira] [Commented] (KAFKA-16298) Ensure user callbacks exceptions are propagated to the user on consumer poll
[ https://issues.apache.org/jira/browse/KAFKA-16298?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17822804#comment-17822804 ] Chia Chuan Yu commented on KAFKA-16298: --- Hi, [~lianetm] I'm a new contributor and I would like to take this one. Is this still available? Thanks! > Ensure user callbacks exceptions are propagated to the user on consumer poll > > > Key: KAFKA-16298 > URL: https://issues.apache.org/jira/browse/KAFKA-16298 > Project: Kafka > Issue Type: Sub-task > Components: clients, consumer >Affects Versions: 3.7.0 >Reporter: Lianet Magrans >Priority: Blocker > Labels: callback, kip-848-client-support > Fix For: 3.8.0 > > > When user-defined callbacks fail with an exception, the expectation is that > the error should be propagated to the user as a KafkaExpception and break the > poll loop (behaviour in the legacy coordinator). The new consumer executes > callbacks in the application thread, and sends an event to the background > with the callback result and error if any, [passing the error along with the > event > here|https://github.com/apache/kafka/blob/98a658f871fc2c533b16fb5fd567a5ceb1c340b7/clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java#L1882] > to the background thread, but does not seem to propagate the exception to > the user. -- This message was sent by Atlassian Jira (v8.20.10#820010)
Re: [PR] KAFKA-16209 : fetchSnapshot might return null if topic is created before v2.8 [kafka]
chiacyu commented on PR #15444: URL: https://github.com/apache/kafka/pull/15444#issuecomment-1974768078 Hi, @showuon Just add a unit test for the condition of the empty snapshot. Thanks! -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
