-
Notifications
You must be signed in to change notification settings - Fork 14.5k
MINOR: Cleanup Trogdor Module #20214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sjhajharia for this patch, left some comments
trogdor/src/main/java/org/apache/kafka/trogdor/coordinator/CoordinatorClient.java
Outdated
Show resolved
Hide resolved
trogdor/src/test/java/org/apache/kafka/trogdor/workload/PayloadGeneratorTest.java
Outdated
Show resolved
Hide resolved
trogdor/src/test/java/org/apache/kafka/trogdor/workload/PayloadGeneratorTest.java
Outdated
Show resolved
Hide resolved
trogdor/src/test/java/org/apache/kafka/trogdor/workload/PayloadGeneratorTest.java
Outdated
Show resolved
Hide resolved
Thank you @m1a2st and @Pankraz76 for the reviews. I have addressed them. |
@JsonCreator | ||
public ErrorResponse(@JsonProperty("code") int code, | ||
@JsonProperty("message") String message) { | ||
this.code = code; | ||
this.message = message; | ||
} | ||
|
||
@Override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nicely done, thanks. Until taken care by plugin it can happen anytime leaving this task kind of open. Offering to apply thins kind of conventions via rewrite.
Adding @chia7712 for review |
@JsonCreator | ||
public TaskRequest(@JsonProperty("taskId") String taskId) { | ||
this.taskId = taskId == null ? "" : taskId; | ||
} | ||
|
||
@Override | ||
@JsonProperty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the name taskId
is same as the method name, so are those annotations really necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
considering Override
kind of best practice, it should not do any harm.
Can not tell anything about JsonProperty
, except of assuming its needed by the business case, therefore covered by some test, or it can be removed/challenged.
Now that Kafka support Java 17, this PR makes some changes in
trogdor
module. The changes mostly include:
Some minor cleanups around use of enhanced switch blocks and conversion of classes to record classes.