cadonna commented on a change in pull request #10428:
URL: https://github.com/apache/kafka/pull/10428#discussion_r614601622
##########
File path: core/src/main/scala/kafka/tools/StreamsResetter.java
##########
@@ -192,64 +194,77 @@ private void maybeDeleteActiveConsumers(final String
groupId,
if (!members.isEmpty()) {
if (options.has(forceOption)) {
System.out.println("Force deleting all active members in the
group: " + groupId);
- adminClient.removeMembersFromConsumerGroup(groupId, new
RemoveMembersFromConsumerGroupOptions()).all().get();
+ adminClient.removeMembersFromConsumerGroup(groupId, new
RemoveMembersFromConsumerGroupOptions()).all()
+ .get();
Review comment:
Although you removed the line length limit, the formatter still breaks
here. Is 120 maybe the default of the eclipse formatter? Maybe it would make
sense to set the limit to a high value (like 200) and decide case by case where
to break as we actually do now.
##########
File path: core/src/test/java/kafka/test/MockController.java
##########
@@ -196,27 +189,24 @@ private MockController(Collection<MockTopic>
initialTopics) {
}
@Override
- public CompletableFuture<Map<ConfigResource, ApiError>>
incrementalAlterConfigs(
- Map<ConfigResource, Map<String, Map.Entry<AlterConfigOp.OpType,
String>>> configChanges,
- boolean validateOnly) {
+ public CompletableFuture<Map<ConfigResource, ApiError>>
incrementalAlterConfigs(Map<ConfigResource, Map<String,
Map.Entry<AlterConfigOp.OpType, String>>> configChanges,
+
boolean validateOnly) {
Review comment:
IMO, this would be better readable as
```suggestion
public CompletableFuture<Map<ConfigResource, ApiError>>
incrementalAlterConfigs(Map<ConfigResource,
Map<String, Map.Entry<AlterConfigOp.OpType, String>>> configChanges,
boolean validateOnly) {
```
(Note: GitHub suggestions make it actually less readable 😅. Each parameter
should be on its own line (except the first) and be aligned after the `(` of
the method.)
##########
File path: core/src/main/scala/kafka/tools/StreamsResetter.java
##########
@@ -192,64 +194,77 @@ private void maybeDeleteActiveConsumers(final String
groupId,
if (!members.isEmpty()) {
if (options.has(forceOption)) {
System.out.println("Force deleting all active members in the
group: " + groupId);
- adminClient.removeMembersFromConsumerGroup(groupId, new
RemoveMembersFromConsumerGroupOptions()).all().get();
+ adminClient.removeMembersFromConsumerGroup(groupId, new
RemoveMembersFromConsumerGroupOptions()).all()
+ .get();
} else {
throw new IllegalStateException("Consumer group '" + groupId +
"' is still active "
- + "and has following members: " + members + ". "
- + "Make sure to stop all running application instances
before running the reset tool."
- + " You can use option '--force' to remove active
members from the group.");
+ + "and has following members: " + members + ". "
+ + "Make sure to stop all running application instances
before running the reset tool."
+ + " You can use option '--force' to remove active members
from the group.");
}
}
}
private void parseArguments(final String[] args) {
final OptionParser optionParser = new OptionParser(false);
- applicationIdOption = optionParser.accepts("application-id", "The
Kafka Streams application ID (application.id).")
- .withRequiredArg()
- .ofType(String.class)
- .describedAs("id")
- .required();
- bootstrapServerOption = optionParser.accepts("bootstrap-servers",
"Comma-separated list of broker urls with format: HOST1:PORT1,HOST2:PORT2")
+ applicationIdOption =
+ optionParser.accepts("application-id", "The Kafka Streams
application ID (application.id).")
+ .withRequiredArg()
+ .ofType(String.class)
+ .describedAs("id")
+ .required();
+ bootstrapServerOption = optionParser
+ .accepts("bootstrap-servers", "Comma-separated list of broker urls
with format: HOST1:PORT1,HOST2:PORT2")
.withRequiredArg()
.ofType(String.class)
.defaultsTo("localhost:9092")
.describedAs("urls");
- inputTopicsOption = optionParser.accepts("input-topics",
"Comma-separated list of user input topics. For these topics, the tool will
reset the offset to the earliest available offset.")
+ inputTopicsOption = optionParser.accepts("input-topics",
+ "Comma-separated list of user input topics. For these topics, the
tool will reset the offset to the earliest available offset.")
.withRequiredArg()
.ofType(String.class)
.withValuesSeparatedBy(',')
.describedAs("list");
- intermediateTopicsOption = optionParser.accepts("intermediate-topics",
"Comma-separated list of intermediate user topics (topics that are input and
output topics, e.g., used in the deprecated through() method). For these
topics, the tool will skip to the end.")
+ intermediateTopicsOption = optionParser.accepts("intermediate-topics",
+ "Comma-separated list of intermediate user topics (topics that are
input and output topics, e.g., used in the deprecated through() method). For
these topics, the tool will skip to the end.")
Review comment:
We (Streams) usually break the arguments of a function call as follows:
```suggestion
intermediateTopicsOption = optionParser.accepts(
"intermediate-topics",
"Comma-separated list of intermediate user topics (topics that
are input and output topics, e.g., used in the deprecated through() method).
For these topics, the tool will skip to the end."
)
```
or
```suggestion
intermediateTopicsOption = optionParser.accepts(
"intermediate-topics",
"Comma-separated list of intermediate user topics (topics that
are input and output topics, e.g., used in the deprecated through() method).
For these topics, the tool will skip to the end.")
```
IMO both are better readable than having arguments filling up the line and
then break. If all arguments fit in one line I would not break them. So my
preferred formatting would be either all arguments fit in one line or each
argument on its own line.
##########
File path: core/src/test/java/kafka/testkit/KafkaClusterTestKit.java
##########
@@ -95,11 +96,9 @@
synchronized void registerPort(int nodeId, int port) {
controllerPorts.put(nodeId, port);
if (controllerPorts.size() >= expectedControllers) {
- future.complete(controllerPorts.entrySet().stream().
- collect(Collectors.toMap(
- Map.Entry::getKey,
- entry -> new RaftConfig.InetAddressSpec(new
InetSocketAddress("localhost", entry.getValue()))
- )));
+
future.complete(controllerPorts.entrySet().stream().collect(Collectors.toMap(
+ Map.Entry::getKey,
+ entry -> new RaftConfig.InetAddressSpec(new
InetSocketAddress("localhost", entry.getValue())))));
Review comment:
I think breaking after `stream()` is on purpose here. At least, I use a
similar style when I use Java's Stream API to make the chain of operators more
readable. Would be a pity if the formatter would change it.
##########
File path: eclipse-formatter.xml
##########
@@ -0,0 +1,52 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
Review comment:
Do you still plan to add the license header here?
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
