Re: [PR] MINOR: Replaced Utils.join() with String.join() [kafka]
chiacyu commented on code in PR #15823:
URL: https://github.com/apache/kafka/pull/15823#discussion_r1583157619
##
clients/src/main/java/org/apache/kafka/common/utils/Utils.java:
##
@@ -584,27 +584,6 @@ public static String formatBytes(long bytes) {
}
}
-/**
- * Create a string representation of an array joined by the given separator
- * @param strs The array of items
- * @param separator The separator
- * @return The string representation.
- */
-public static String join(T[] strs, String separator) {
-return join(Arrays.asList(strs), separator);
-}
-
-/**
- * Create a string representation of a collection joined by the given
separator
- * @param collection The list of items
- * @param separator The separator
- * @return The string representation.
- */
-public static String join(Collection collection, String separator) {
-Objects.requireNonNull(collection);
-return mkString(collection.stream(), "", "", separator);
-}
-
/**
* Create a string representation of a stream surrounded by `begin` and
`end` and joined by `separator`.
*
Review Comment:
I looked up the code base and found there're almost dozens use cases of
`Utils.mkString()`. Should we remove that too? If so, is that a better
representation to eliminate it in another PR? 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] MINOR: Replaced Utils.join() with String.join() [kafka]
chia7712 commented on code in PR #15823:
URL: https://github.com/apache/kafka/pull/15823#discussion_r1582290331
##
clients/src/main/java/org/apache/kafka/clients/FetchSessionHandler.java:
##
@@ -392,14 +392,14 @@ private String
topicPartitionsToLogString(Collection partitions)
if (!log.isTraceEnabled()) {
return String.format("%d partition(s)", partitions.size());
}
-return "(" + Utils.join(partitions, ", ") + ")";
+return "(" + String.join(", ", Arrays.toString(partitions.toArray()))
+ ")";
Review Comment:
ditto
##
clients/src/main/java/org/apache/kafka/clients/FetchSessionHandler.java:
##
@@ -392,14 +392,14 @@ private String
topicPartitionsToLogString(Collection partitions)
if (!log.isTraceEnabled()) {
return String.format("%d partition(s)", partitions.size());
}
-return "(" + Utils.join(partitions, ", ") + ")";
+return "(" + String.join(", ", Arrays.toString(partitions.toArray()))
+ ")";
}
private String topicIdPartitionsToLogString(Collection
partitions) {
if (!log.isTraceEnabled()) {
return String.format("%d partition(s)", partitions.size());
}
-return "(" + Utils.join(partitions, ", ") + ")";
+return "(" + String.join(", ", Arrays.toString(partitions.toArray()))
+ ")";
Review Comment:
ditto
##
clients/src/main/java/org/apache/kafka/common/utils/Utils.java:
##
@@ -584,27 +584,6 @@ public static String formatBytes(long bytes) {
}
}
-/**
- * Create a string representation of an array joined by the given separator
- * @param strs The array of items
- * @param separator The separator
- * @return The string representation.
- */
-public static String join(T[] strs, String separator) {
-return join(Arrays.asList(strs), separator);
-}
-
-/**
- * Create a string representation of a collection joined by the given
separator
- * @param collection The list of items
- * @param separator The separator
- * @return The string representation.
- */
-public static String join(Collection collection, String separator) {
-Objects.requireNonNull(collection);
-return mkString(collection.stream(), "", "", separator);
-}
-
/**
* Create a string representation of a stream surrounded by `begin` and
`end` and joined by `separator`.
*
Review Comment:
It seems `mkString(Stream stream, String begin, String end, String
separator)` can be removed too since the only one use case
(https://github.com/apache/kafka/blob/5de5d967adffd864bad3ec729760a430253abf38/tools/src/test/java/org/apache/kafka/tools/consumer/group/ConsumerGroupServiceTest.java#L199)
does not use "begin" and "end"
##
clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java:
##
@@ -131,7 +130,7 @@ public class CommonClientConfigs {
public static final String SECURITY_PROTOCOL_CONFIG = "security.protocol";
public static final String SECURITY_PROTOCOL_DOC = "Protocol used to
communicate with brokers. Valid values are: " +
-Utils.join(SecurityProtocol.names(), ", ") + ".";
+String.join(", ", SecurityProtocol.names()).replace("[",
"").replace("]", "") + ".";
Review Comment:
please don't use `replace`. We can generate the correct message at once.
--
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: Replaced Utils.join() with String.join() [kafka]
chiacyu commented on code in PR #15823:
URL: https://github.com/apache/kafka/pull/15823#discussion_r1582059240
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java:
##
@@ -1491,7 +1490,7 @@ public void assign(Collection partitions)
{
// See the ApplicationEventProcessor.process() method that handles
this event for more detail.
applicationEventHandler.add(new
AssignmentChangeEvent(subscriptions.allConsumed(), time.milliseconds()));
-log.info("Assigned to partition(s): {}", join(partitions, ", "));
+log.info("Assigned to partition(s): {}", String.join(", ",
Arrays.toString(partitions.toArray(;
Review Comment:
Got it, thanks for the advice!
--
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: Replaced Utils.join() with String.join() [kafka]
chia7712 commented on code in PR #15823:
URL: https://github.com/apache/kafka/pull/15823#discussion_r1582049275
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java:
##
@@ -1491,7 +1490,7 @@ public void assign(Collection partitions)
{
// See the ApplicationEventProcessor.process() method that handles
this event for more detail.
applicationEventHandler.add(new
AssignmentChangeEvent(subscriptions.allConsumed(), time.milliseconds()));
-log.info("Assigned to partition(s): {}", join(partitions, ", "));
+log.info("Assigned to partition(s): {}", String.join(", ",
Arrays.toString(partitions.toArray(;
Review Comment:
how about
`partitions.stream().map(TopicPartition::toString).collect(Collectors.joining(","))`?
--
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: Replaced Utils.join() with String.join() [kafka]
chiacyu commented on code in PR #15823:
URL: https://github.com/apache/kafka/pull/15823#discussion_r1581992286
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java:
##
@@ -1491,7 +1490,7 @@ public void assign(Collection partitions)
{
// See the ApplicationEventProcessor.process() method that handles
this event for more detail.
applicationEventHandler.add(new
AssignmentChangeEvent(subscriptions.allConsumed(), time.milliseconds()));
-log.info("Assigned to partition(s): {}", join(partitions, ", "));
+log.info("Assigned to partition(s): {}", String.join(", ",
Arrays.toString(partitions.toArray(;
Review Comment:
I'm thinking to append `replace("[", "").replace("]", "")` to eliminate
brackets. If these's any better representations please let me know. 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] MINOR: Replaced Utils.join() with String.join() [kafka]
chia7712 commented on code in PR #15823:
URL: https://github.com/apache/kafka/pull/15823#discussion_r1581877008
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorPluginsResourceTest.java:
##
@@ -106,7 +106,7 @@ public class ConnectorPluginsResourceTest {
PARTIAL_PROPS.put("name", "test");
PARTIAL_PROPS.put("test.string.config", "testString");
PARTIAL_PROPS.put("test.int.config", "1");
-PARTIAL_PROPS.put("test.list.config", "a,b");
+PARTIAL_PROPS.put("test.list.config", "a, b");
Review Comment:
ditto. please revert unrelated change
##
clients/src/main/java/org/apache/kafka/clients/admin/ConsumerGroupDescription.java:
##
@@ -161,7 +160,7 @@ public Set authorizedOperations() {
public String toString() {
return "(groupId=" + groupId +
", isSimpleConsumerGroup=" + isSimpleConsumerGroup +
-", members=" + Utils.join(members, ",") +
+", members=" + String.join(",",
Arrays.toString(members.toArray())) +
Review Comment:
ditto
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorPluginsResourceTest.java:
##
@@ -188,7 +188,7 @@ public class ConnectorPluginsResourceTest {
partialConfigs.add(configInfo);
configKeyInfo = new ConfigKeyInfo("test.list.config", "LIST", true,
null, "HIGH", "Test configuration for list type.", "Test", 2, "LONG",
"test.list.config", Collections.emptyList());
-configValueInfo = new ConfigValueInfo("test.list.config", "a,b",
asList("a", "b", "c"), Collections.emptyList(), true);
+configValueInfo = new ConfigValueInfo("test.list.config", "a, b",
asList("a", "b", "c"), Collections.emptyList(), true);
Review Comment:
ditto. please revert unrelated change
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java:
##
@@ -1491,7 +1490,7 @@ public void assign(Collection partitions)
{
// See the ApplicationEventProcessor.process() method that handles
this event for more detail.
applicationEventHandler.add(new
AssignmentChangeEvent(subscriptions.allConsumed(), time.milliseconds()));
-log.info("Assigned to partition(s): {}", join(partitions, ", "));
+log.info("Assigned to partition(s): {}", String.join(", ",
Arrays.toString(partitions.toArray(;
Review Comment:
`Arrays.toString` will add `[]` so that is a bit different to origin
message. Maybe you can use the lambda to generate same message.
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/AbstractHerderTest.java:
##
@@ -729,7 +729,7 @@ public void testConfigValidationAllOverride() {
String idempotenceConfigKey =
producerOverrideKey(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG);
config.put(idempotenceConfigKey, "true");
String bootstrapServersConfigKey =
producerOverrideKey(CommonClientConfigs.BOOTSTRAP_SERVERS_CONFIG);
-config.put(bootstrapServersConfigKey,
"SASL_PLAINTEXT://localhost:12345,SASL_PLAINTEXT://localhost:23456");
+config.put(bootstrapServersConfigKey,
"SASL_PLAINTEXT://localhost:12345, SASL_PLAINTEXT://localhost:23456");
Review Comment:
could you please revert this unrelated change?
##
tools/src/main/java/org/apache/kafka/tools/consumer/group/ConsumerGroupCommandOptions.java:
##
@@ -213,11 +211,11 @@ void checkArgs() {
if (options.has(describeOpt)) {
if (!options.has(groupOpt) && !options.has(allGroupsOpt))
CommandLineUtils.printUsageAndExit(parser,
-"Option " + describeOpt + " takes one of these options: "
+ join(allGroupSelectionScopeOpts, ", "));
+"Option " + describeOpt + " takes one of these options: "
+ String.join(", ", Arrays.toString(allGroupSelectionScopeOpts.toArray(;
Review Comment:
ditto
##
tools/src/main/java/org/apache/kafka/tools/consumer/group/ConsumerGroupCommandOptions.java:
##
@@ -230,7 +228,7 @@ void checkArgs() {
if (options.has(deleteOpt)) {
if (!options.has(groupOpt) && !options.has(allGroupsOpt))
CommandLineUtils.printUsageAndExit(parser,
-"Option " + deleteOpt + " takes one of these options: " +
join(allGroupSelectionScopeOpts, ", "));
+"Option " + deleteOpt + " takes one of these options: " +
String.join(", ", Arrays.toString(allGroupSelectionScopeOpts.toArray(;
Review Comment:
ditto
##
tools/src/main/java/org/apache/kafka/tools/consumer/group/ConsumerGroupCommand.java:
##
@@ -132,7 +132,7 @@ static Set
consumerGroupStatesFromString(String input) {
Set parsedStates =
Arrays.stream(input.split(",")).map(s ->
ConsumerGroupState.parse(s.trim())).collect(Collectors.toSet());
if (parsedStates.c
