You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "sjhajharia (via GitHub)" <gi...@apache.org> on 2025/08/10 07:06:44 UTC
[PR] MINOR: Cleanups in Test Module (3/n) [kafka]
sjhajharia opened a new pull request, #20332:
URL: https://github.com/apache/kafka/pull/20332
This PR aims at cleaning up the tools module further by getting rid of some extra code which can be replaced by `record`
--
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: Cleanups in Tools Module (3/n) [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #20332:
URL: https://github.com/apache/kafka/pull/20332#issuecomment-3194996626
A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the `triage` label
should be removed to prevent this automation from happening again.
--
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: Cleanups in Tools Module (3/n) [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20332:
URL: https://github.com/apache/kafka/pull/20332#discussion_r2283709791
##########
tools/src/main/java/org/apache/kafka/tools/reassign/CompletedMoveState.java:
##########
@@ -17,36 +17,19 @@
package org.apache.kafka.tools.reassign;
-import java.util.Objects;
-
/**
* The completed replica log directory move state.
*/
-final class CompletedMoveState implements LogDirMoveState {
- public final String targetLogDir;
-
+record CompletedMoveState(String targetLogDir) implements LogDirMoveState {
/**
- * @param targetLogDir The log directory that we wanted the replica to move to.
+ * @param targetLogDir The log directory that we wanted the replica to move to.
*/
- public CompletedMoveState(String targetLogDir) {
- this.targetLogDir = targetLogDir;
+ CompletedMoveState {
Review Comment:
ditto
##########
tools/src/main/java/org/apache/kafka/tools/AclCommand.java:
##########
@@ -102,13 +102,7 @@ private static Properties adminConfigs(AclCommandOptions opts) throws IOExceptio
return props;
}
- private static class AdminClientService {
-
- private final AclCommandOptions opts;
-
- AdminClientService(AclCommandOptions opts) {
- this.opts = opts;
- }
+ private record AdminClientService(AclCommandOptions opts) {
Review Comment:
Rewriting it as static helper methods seems to better than using a `record` class. WDYT?
##########
tools/src/main/java/org/apache/kafka/tools/reassign/ActiveMoveState.java:
##########
@@ -17,44 +17,21 @@
package org.apache.kafka.tools.reassign;
-import java.util.Objects;
-
/**
* A replica log directory move state where the move is in progress.
*/
-final class ActiveMoveState implements LogDirMoveState {
- public final String currentLogDir;
-
- public final String targetLogDir;
-
- public final String futureLogDir;
-
+record ActiveMoveState(String currentLogDir, String targetLogDir, String futureLogDir) implements LogDirMoveState {
/**
- * @param currentLogDir The current log directory.
- * @param futureLogDir The log directory that the replica is moving to.
- * @param targetLogDir The log directory that we wanted the replica to move to.
+ * @param currentLogDir The current log directory.
+ * @param futureLogDir The log directory that the replica is moving to.
+ * @param targetLogDir The log directory that we wanted the replica to move to.
*/
- public ActiveMoveState(String currentLogDir, String targetLogDir, String futureLogDir) {
- this.currentLogDir = currentLogDir;
- this.targetLogDir = targetLogDir;
- this.futureLogDir = futureLogDir;
+ ActiveMoveState {
Review Comment:
this is useless, right?
##########
tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java:
##########
@@ -196,22 +196,8 @@ enum Command {
LIST, SYNC_MANIFESTS
}
- private static class Config {
- private final Command command;
- private final Set<Path> locations;
- private final boolean dryRun;
- private final boolean keepNotFound;
- private final PrintStream out;
- private final PrintStream err;
-
- private Config(Command command, Set<Path> locations, boolean dryRun, boolean keepNotFound, PrintStream out, PrintStream err) {
- this.command = command;
- this.locations = locations;
- this.dryRun = dryRun;
- this.keepNotFound = keepNotFound;
- this.out = out;
- this.err = err;
- }
+ private record Config(Command command, Set<Path> locations, boolean dryRun, boolean keepNotFound, PrintStream out,
+ PrintStream err) {
@Override
public String toString() {
Review Comment:
how about trusting the generated `toString`?
##########
tools/src/main/java/org/apache/kafka/tools/TransactionsCommand.java:
##########
@@ -848,17 +847,7 @@ private List<OpenTransaction> collectCandidateOpenTransactions(
return candidateTransactions;
}
- private static class OpenTransaction {
- private final TopicPartition topicPartition;
- private final ProducerState producerState;
-
- private OpenTransaction(
- TopicPartition topicPartition,
- ProducerState producerState
- ) {
- this.topicPartition = topicPartition;
- this.producerState = producerState;
- }
+ private record OpenTransaction(TopicPartition topicPartition, ProducerState producerState) {
Review Comment:
Based on the use cases, using `Map<TopicPartition, ProducerState>` seems more suitable.
##########
tools/src/test/java/org/apache/kafka/tools/ConnectPluginPathTest.java:
##########
@@ -444,13 +444,7 @@ private enum PluginLocationType {
MULTI_JAR
}
- private static class PluginLocation {
- private final Path path;
-
- private PluginLocation(Path path) {
- this.path = path;
- }
-
+ private record PluginLocation(Path path) {
@Override
public String toString() {
Review Comment:
ditto
##########
tools/src/main/java/org/apache/kafka/tools/reassign/MissingReplicaMoveState.java:
##########
@@ -17,36 +17,18 @@
package org.apache.kafka.tools.reassign;
-import java.util.Objects;
-
/**
* A replica log directory move state where the source log directory is missing.
*/
-final class MissingReplicaMoveState implements LogDirMoveState {
- public final String targetLogDir;
-
+record MissingReplicaMoveState(String targetLogDir) implements LogDirMoveState {
/**
- * @param targetLogDir The log directory that we wanted the replica to move to.
+ * @param targetLogDir The log directory that we wanted the replica to move to.
*/
- public MissingReplicaMoveState(String targetLogDir) {
- this.targetLogDir = targetLogDir;
+ MissingReplicaMoveState {
Review Comment:
ditto
##########
tools/src/main/java/org/apache/kafka/tools/reassign/MissingLogDirMoveState.java:
##########
@@ -17,36 +17,18 @@
package org.apache.kafka.tools.reassign;
-import java.util.Objects;
-
/**
* A replica log directory move state where the source replica is missing.
*/
-final class MissingLogDirMoveState implements LogDirMoveState {
- public final String targetLogDir;
-
+record MissingLogDirMoveState(String targetLogDir) implements LogDirMoveState {
/**
- * @param targetLogDir The log directory that we wanted the replica to move to.
+ * @param targetLogDir The log directory that we wanted the replica to move to.
*/
- public MissingLogDirMoveState(String targetLogDir) {
- this.targetLogDir = targetLogDir;
+ MissingLogDirMoveState {
Review Comment:
ditto
##########
tools/src/main/java/org/apache/kafka/tools/reassign/CancelledMoveState.java:
##########
@@ -17,41 +17,21 @@
package org.apache.kafka.tools.reassign;
-import java.util.Objects;
-
/**
* A replica log directory move state where there is no move in progress, but we did not
* reach the target log directory.
*/
-final class CancelledMoveState implements LogDirMoveState {
- public final String currentLogDir;
-
- public final String targetLogDir;
-
+record CancelledMoveState(String currentLogDir, String targetLogDir) implements LogDirMoveState {
/**
- * @param currentLogDir The current log directory.
- * @param targetLogDir The log directory that we wanted the replica to move to.
+ * @param currentLogDir The current log directory.
+ * @param targetLogDir The log directory that we wanted the replica to move to.
*/
- public CancelledMoveState(String currentLogDir, String targetLogDir) {
- this.currentLogDir = currentLogDir;
- this.targetLogDir = targetLogDir;
+ CancelledMoveState {
Review Comment:
ditto
--
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: Cleanups in Test Module (3/n) [kafka]
Posted by "sjhajharia (via GitHub)" <gi...@apache.org>.
sjhajharia commented on PR #20332:
URL: https://github.com/apache/kafka/pull/20332#issuecomment-3173936057
Adding @chia7712 as he kindly helped with all PR reviews in this region.
--
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: Cleanups in Tools Module (3/n) [kafka]
Posted by "sjhajharia (via GitHub)" <gi...@apache.org>.
sjhajharia commented on code in PR #20332:
URL: https://github.com/apache/kafka/pull/20332#discussion_r2284036840
##########
tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java:
##########
@@ -196,22 +196,8 @@ enum Command {
LIST, SYNC_MANIFESTS
}
- private static class Config {
- private final Command command;
- private final Set<Path> locations;
- private final boolean dryRun;
- private final boolean keepNotFound;
- private final PrintStream out;
- private final PrintStream err;
-
- private Config(Command command, Set<Path> locations, boolean dryRun, boolean keepNotFound, PrintStream out, PrintStream err) {
- this.command = command;
- this.locations = locations;
- this.dryRun = dryRun;
- this.keepNotFound = keepNotFound;
- this.out = out;
- this.err = err;
- }
+ private record Config(Command command, Set<Path> locations, boolean dryRun, boolean keepNotFound, PrintStream out,
+ PrintStream err) {
@Override
public String toString() {
Review Comment:
The auto generated one will also have the `out` and `err` params which the record takes in. Hence I prefered keeping this custom one.
##########
tools/src/main/java/org/apache/kafka/tools/TransactionsCommand.java:
##########
@@ -848,17 +847,7 @@ private List<OpenTransaction> collectCandidateOpenTransactions(
return candidateTransactions;
}
- private static class OpenTransaction {
- private final TopicPartition topicPartition;
- private final ProducerState producerState;
-
- private OpenTransaction(
- TopicPartition topicPartition,
- ProducerState producerState
- ) {
- this.topicPartition = topicPartition;
- this.producerState = producerState;
- }
+ private record OpenTransaction(TopicPartition topicPartition, ProducerState producerState) {
Review Comment:
Sorry, but I dont cpmpletely follow here. Do we want to change the way `OpenTransaction` is defined?
If so, can we create a ticket and deal with the same separately as it may cause some additional changes in this file.
##########
tools/src/test/java/org/apache/kafka/tools/ConnectPluginPathTest.java:
##########
@@ -444,13 +444,7 @@ private enum PluginLocationType {
MULTI_JAR
}
- private static class PluginLocation {
- private final Path path;
-
- private PluginLocation(Path path) {
- this.path = path;
- }
-
+ private record PluginLocation(Path path) {
@Override
public String toString() {
Review Comment:
I don't think we can get rid of this toString() method.
The default Java toString would create a value like "PluginLocation[path=...]", but the expectation here is to just have the path and not the name of the class.
--
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: Cleanups in Tools Module (3/n) [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20332:
URL: https://github.com/apache/kafka/pull/20332#discussion_r2285181152
##########
tools/src/main/java/org/apache/kafka/tools/TransactionsCommand.java:
##########
@@ -848,17 +847,7 @@ private List<OpenTransaction> collectCandidateOpenTransactions(
return candidateTransactions;
}
- private static class OpenTransaction {
- private final TopicPartition topicPartition;
- private final ProducerState producerState;
-
- private OpenTransaction(
- TopicPartition topicPartition,
- ProducerState producerState
- ) {
- this.topicPartition = topicPartition;
- this.producerState = producerState;
- }
+ private record OpenTransaction(TopicPartition topicPartition, ProducerState producerState) {
Review Comment:
yes, I will create a follow-up for 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
Re: [PR] MINOR: Cleanups in Tools Module (3/n) [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #20332:
URL: https://github.com/apache/kafka/pull/20332
--
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