MINOR: Implement toStringBase to avoid duplicated code #14391
Conversation
|
|
||
| protected ApplicationEvent(Type type) { | ||
| protected ApplicationEvent(Type type, String owner) { |
There was a problem hiding this comment.
As a simplification, this is not needed I would say. We could just remove the owner var and constructor param, and leave only a simple toStringBase like this:
protected String toStringBase() {
return "owner='" + getClass().getSimpleName() + '\'' +
", type=" + type;
}
It would end up returning the simple name of the child class, which is what we want in the end.
There was a problem hiding this comment.
maybe we could get rid of owner after all, and just have the calling class to print its name so that we could follow a more conventional toString(). Here's an example of how it might look like after getting rid of owner. Would this work?
ListOffsetsApplicationEvent{type=LIST_OFFSETS, future=java.util.concurrent.CompletableFuture@bdd2027[Not completed], timestampsToSearch={}, requireTimestamps=false}
There was a problem hiding this comment.
Sure, the point was to avoid the duplicated info in the toString, result of including the class name in the base class and in the child implementations. See my previous comment related to this down below.
| @@ -26,8 +26,17 @@ public class AssignmentChangeApplicationEvent extends ApplicationEvent { | |||
| final long currentTimeMs; | |||
|
|
|||
| public AssignmentChangeApplicationEvent(final Map<TopicPartition, OffsetAndMetadata> offsets, final long currentTimeMs) { | |||
| super(Type.ASSIGNMENT_CHANGE); | |||
| super(Type.ASSIGNMENT_CHANGE, AssignmentChangeApplicationEvent.class.getSimpleName()); | |||
There was a problem hiding this comment.
This would then change back to how it was if we agree on using the getClass().getSimpleName() in the toStringBase
| return getClass().getSimpleName() + " {" + | ||
| "timestampsToSearch=" + timestampsToSearch + ", " + | ||
| "requireTimestamps=" + requireTimestamps + '}'; | ||
| return "ListOffsetsApplicationEvent{" + |
There was a problem hiding this comment.
Is it intentional to explicitly include "ListOffsetsApplicationEvent" here? it is in the "owner" section of the toStringBase too.
There was a problem hiding this comment.
(Similar for all other events that are explicitly including the class name and then calling the toStringBase)
|
Apparently @kirktrue implemented those in his previous commit. So closing this. |
This is a follow up of #14386.
The goal of this PR is to refactor the toString method to reduce duplicated code.