You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "yunchipang (via GitHub)" <gi...@apache.org> on 2025/08/18 23:56:38 UTC
[PR] MINOR: Change LogCompactionTester topics parameter to Set [kafka]
yunchipang opened a new pull request, #20372:
URL: https://github.com/apache/kafka/pull/20372
issue: https://github.com/apache/kafka/pull/19905#discussion_r2282202312
What: Change `String[] topics` to `Set<String> topics` throughout `LogCompactionTester`.
Why: `Set<String>` is more modern and reduces the need for array-to-collection conversions.
--
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: Change LogCompactionTester topics parameter to Set [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on code in PR #20372:
URL: https://github.com/apache/kafka/pull/20372#discussion_r2284282535
##########
tools/src/main/java/org/apache/kafka/tools/LogCompactionTester.java:
##########
@@ -408,8 +410,9 @@ producerProps, new ByteArraySerializer(), new ByteArraySerializer())) {
try (BufferedWriter producedWriter = Files.newBufferedWriter(
producedFilePath, StandardCharsets.UTF_8)) {
- for (long i = 0; i < messages * topics.length; i++) {
- String topic = topics[(int) (i % topics.length)];
+ String[] topicsArray = topics.toArray(new String[0]);
Review Comment:
+1
--
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: Change LogCompactionTester topics parameter to Set [kafka]
Posted by "m1a2st (via GitHub)" <gi...@apache.org>.
m1a2st commented on code in PR #20372:
URL: https://github.com/apache/kafka/pull/20372#discussion_r2283811015
##########
tools/src/main/java/org/apache/kafka/tools/LogCompactionTester.java:
##########
@@ -246,9 +248,9 @@ public static void main(String[] args) throws Exception {
int sleepSecs = optionSet.valueOf(options.sleepSecsOpt);
long testId = RANDOM.nextLong();
- String[] topics = IntStream.range(0, topicCount)
+ Set<String> topics = IntStream.range(0, topicCount)
.mapToObj(i -> "log-cleaner-test-" + testId + "-" + i)
- .toArray(String[]::new);
+ .collect(toCollection(LinkedHashSet::new));
Review Comment:
Why don’t we use a `List`? I think a `List` is more convenient than a set when we need to retrieve elements.
--
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: Change LogCompactionTester topics parameter to Set [kafka]
Posted by "m1a2st (via GitHub)" <gi...@apache.org>.
m1a2st commented on code in PR #20372:
URL: https://github.com/apache/kafka/pull/20372#discussion_r2283811015
##########
tools/src/main/java/org/apache/kafka/tools/LogCompactionTester.java:
##########
@@ -246,9 +248,9 @@ public static void main(String[] args) throws Exception {
int sleepSecs = optionSet.valueOf(options.sleepSecsOpt);
long testId = RANDOM.nextLong();
- String[] topics = IntStream.range(0, topicCount)
+ Set<String> topics = IntStream.range(0, topicCount)
.mapToObj(i -> "log-cleaner-test-" + testId + "-" + i)
- .toArray(String[]::new);
+ .collect(toCollection(LinkedHashSet::new));
Review Comment:
Why don’t we use a `List`? I think a `List` is more convenient than a `Set` when we need to retrieve elements.
--
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: Change LogCompactionTester topics parameter to Set [kafka]
Posted by "yunchipang (via GitHub)" <gi...@apache.org>.
yunchipang commented on PR #20372:
URL: https://github.com/apache/kafka/pull/20372#issuecomment-3198814856
test log
```
SLF4J: Class path contains multiple SLF4J bindings.
SLF4J: Found binding in [jar:file:/opt/kafka-dev/core/build/dependant-testlibs/log4j-slf4j-impl-2.24.3.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: Found binding in [jar:file:/opt/kafka-dev/tools/build/dependant-libs-2.13.16/log4j-slf4j-impl-2.24.3.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: See http://www.slf4j.org/codes.html#multiple_bindings for an explanation.
SLF4J: Actual binding is of type [org.apache.logging.slf4j.Log4jLoggerFactory]
Producing 1000000 messages..to topics log-cleaner-test--5661911372922944212-0
Logging produce requests to /tmp/kafka-log-cleaner-produced-13037492074713603521.txt
Sleeping for 20seconds...
Consuming messages...
Logging consumed messages to /tmp/kafka-log-cleaner-consumed-6843967367886408887.txt
1000000 rows of data produced, 120183 rows of data consumed (88.0% reduction).
De-duplicating and validating output files...
Validated 89943 values, 0 mismatches.
Data verification is completed
```
--
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: Change LogCompactionTester topics parameter to Set [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #20372:
URL: https://github.com/apache/kafka/pull/20372
--
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: Change LogCompactionTester topics parameter to Set [kafka]
Posted by "Yunyung (via GitHub)" <gi...@apache.org>.
Yunyung commented on code in PR #20372:
URL: https://github.com/apache/kafka/pull/20372#discussion_r2283783540
##########
tools/src/main/java/org/apache/kafka/tools/LogCompactionTester.java:
##########
@@ -408,8 +410,9 @@ producerProps, new ByteArraySerializer(), new ByteArraySerializer())) {
try (BufferedWriter producedWriter = Files.newBufferedWriter(
producedFilePath, StandardCharsets.UTF_8)) {
- for (long i = 0; i < messages * topics.length; i++) {
- String topic = topics[(int) (i % topics.length)];
+ String[] topicsArray = topics.toArray(new String[0]);
Review Comment:
How about
List<String> topicsList = List.copyOf(topics);
...
String topic = topicList.get((int) (i % size));
##########
tools/src/main/java/org/apache/kafka/tools/LogCompactionTester.java:
##########
@@ -430,14 +433,14 @@ record = new ProducerRecord<>(topic,
}
}
- private static Path consumeMessages(String brokerUrl, String[] topics) throws IOException {
+ private static Path consumeMessages(String brokerUrl, Set<String> topics) throws IOException {
Path consumedFilePath = Files.createTempFile("kafka-log-cleaner-consumed-", ".txt");
System.out.println("Logging consumed messages to " + consumedFilePath);
try (Consumer<String, String> consumer = createConsumer(brokerUrl);
BufferedWriter consumedWriter = Files.newBufferedWriter(consumedFilePath, StandardCharsets.UTF_8)) {
- consumer.subscribe(Arrays.asList(topics));
+ consumer.subscribe(new ArrayList<>(topics));
Review Comment:
We can use `consumer.subscribe(topics)` directly;
--
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: Change LogCompactionTester topics parameter to Set [kafka]
Posted by "yunchipang (via GitHub)" <gi...@apache.org>.
yunchipang commented on code in PR #20372:
URL: https://github.com/apache/kafka/pull/20372#discussion_r2292251480
##########
tools/src/main/java/org/apache/kafka/tools/LogCompactionTester.java:
##########
@@ -430,14 +433,14 @@ record = new ProducerRecord<>(topic,
}
}
- private static Path consumeMessages(String brokerUrl, String[] topics) throws IOException {
+ private static Path consumeMessages(String brokerUrl, Set<String> topics) throws IOException {
Path consumedFilePath = Files.createTempFile("kafka-log-cleaner-consumed-", ".txt");
System.out.println("Logging consumed messages to " + consumedFilePath);
try (Consumer<String, String> consumer = createConsumer(brokerUrl);
BufferedWriter consumedWriter = Files.newBufferedWriter(consumedFilePath, StandardCharsets.UTF_8)) {
- consumer.subscribe(Arrays.asList(topics));
+ consumer.subscribe(new ArrayList<>(topics));
Review Comment:
fixed!
##########
tools/src/main/java/org/apache/kafka/tools/LogCompactionTester.java:
##########
@@ -408,8 +410,9 @@ producerProps, new ByteArraySerializer(), new ByteArraySerializer())) {
try (BufferedWriter producedWriter = Files.newBufferedWriter(
producedFilePath, StandardCharsets.UTF_8)) {
- for (long i = 0; i < messages * topics.length; i++) {
- String topic = topics[(int) (i % topics.length)];
+ String[] topicsArray = topics.toArray(new String[0]);
Review Comment:
fixed!
--
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: Change LogCompactionTester topics parameter to Set [kafka]
Posted by "yunchipang (via GitHub)" <gi...@apache.org>.
yunchipang commented on code in PR #20372:
URL: https://github.com/apache/kafka/pull/20372#discussion_r2292283432
##########
tools/src/main/java/org/apache/kafka/tools/LogCompactionTester.java:
##########
@@ -246,9 +248,9 @@ public static void main(String[] args) throws Exception {
int sleepSecs = optionSet.valueOf(options.sleepSecsOpt);
long testId = RANDOM.nextLong();
- String[] topics = IntStream.range(0, topicCount)
+ Set<String> topics = IntStream.range(0, topicCount)
.mapToObj(i -> "log-cleaner-test-" + testId + "-" + i)
- .toArray(String[]::new);
+ .collect(toCollection(LinkedHashSet::new));
Review Comment:
@m1a2st thanks for the review! I totally agree with `List` would be more convenient for indexed access, but `Set` is preferred here because
1. semantic clarity: `topics` represents a collection of unique topic names
2. duplicate prevention: `Set` protects against duplicate topics by accident
--
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