Skip to content

MINOR: Implement toStringBase to avoid duplicated code #14391

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.

Sign up for GitHub

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

Closed
philipnee wants to merge 2 commits into from
Closed

MINOR: Implement toStringBase to avoid duplicated code #14391

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c06adc
Implement toStringBase to avoid duplicated code
Sep 14, 2023
d58dc9e
based on pr comment
Sep 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions ...ts/src/main/java/org/apache/kafka/clients/consumer/internals/events/ApplicationEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,27 @@
*/
abstract public class ApplicationEvent {
public final Type type;
protected final String owner;

protected ApplicationEvent(Type type) {
protected ApplicationEvent(Type type, String owner) {

@lianetm lianetm Sep 15, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@philipnee philipnee Sep 18, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}

@lianetm lianetm Sep 18, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.type = type;
this.owner = owner;
}

@Override
public String toString() {
return type + " ApplicationEvent";
return "ApplicationEvent{" +
toStringBase() +
'}';
}

public enum Type {
NOOP, COMMIT, POLL, FETCH_COMMITTED_OFFSET, METADATA_UPDATE, ASSIGNMENT_CHANGE,
LIST_OFFSETS, RESET_POSITIONS, VALIDATE_POSITIONS,
}

protected String toStringBase() {
return "owner='" + owner + '\'' +
", type=" + type;
}
}
11 changes: 10 additions & 1 deletion .../org/apache/kafka/clients/consumer/internals/events/AssignmentChangeApplicationEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());

@lianetm lianetm Sep 15, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would then change back to how it was if we agree on using the getClass().getSimpleName() in the toStringBase

this.offsets = offsets;
this.currentTimeMs = currentTimeMs;
}

@Override
public String toString() {
return "AssignmentChangeApplicationEvent{" +
toStringBase() +
", offsets=" + offsets +
", currentTimeMs=" + currentTimeMs +
'}';
}
}
8 changes: 5 additions & 3 deletions .../main/java/org/apache/kafka/clients/consumer/internals/events/CommitApplicationEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public class CommitApplicationEvent extends ApplicationEvent {
final private Map<TopicPartition, OffsetAndMetadata> offsets;

public CommitApplicationEvent(final Map<TopicPartition, OffsetAndMetadata> offsets) {
super(Type.COMMIT);
super(Type.COMMIT, CommitApplicationEvent.class.getSimpleName());
this.offsets = offsets;
Optional<Exception> exception = isValid(offsets);
if (exception.isPresent()) {
Expand Down Expand Up @@ -58,7 +58,9 @@ private Optional<Exception> isValid(final Map<TopicPartition, OffsetAndMetadata>

@Override
public String toString() {
return "CommitApplicationEvent("
+ "offsets=" + offsets + ")";
return "CommitApplicationEvent{" +
toStringBase() +
", offsets=" + offsets +
'}';
}
}
17 changes: 11 additions & 6 deletions .../java/org/apache/kafka/clients/consumer/internals/events/CompletableApplicationEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ public abstract class CompletableApplicationEvent<T> extends ApplicationEvent {

protected final CompletableFuture<T> future;

protected CompletableApplicationEvent(Type type) {
super(type);
protected CompletableApplicationEvent(Type type, String owner) {
super(type, owner);
this.future = new CompletableFuture<>();
}

Expand Down Expand Up @@ -89,11 +89,16 @@ public int hashCode() {
return result;
}

@Override
public String toStringBase() {
return super.toStringBase() +
", future=" + future;
}

@Override
public String toString() {
return getClass().getSimpleName() + "{" +
"future=" + future +
", type=" + type +
'}';
return "CompletableApplicationEvent{" +
toStringBase() +
'}';
}
}
11 changes: 6 additions & 5 deletions .../java/org/apache/kafka/clients/consumer/internals/events/ListOffsetsApplicationEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public class ListOffsetsApplicationEvent extends CompletableApplicationEvent<Map
private final boolean requireTimestamps;

public ListOffsetsApplicationEvent(Map<TopicPartition, Long> timestampToSearch, boolean requireTimestamps) {
super(Type.LIST_OFFSETS);
super(Type.LIST_OFFSETS, ListOffsetsApplicationEvent.class.getSimpleName());
this.timestampsToSearch = Collections.unmodifiableMap(timestampToSearch);
this.requireTimestamps = requireTimestamps;
}
Expand Down Expand Up @@ -85,9 +85,10 @@ public int hashCode() {

@Override
public String toString() {
return getClass().getSimpleName() + " {" +
"timestampsToSearch=" + timestampsToSearch + ", " +
"requireTimestamps=" + requireTimestamps + '}';
return "ListOffsetsApplicationEvent{" +

@lianetm lianetm Sep 15, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional to explicitly include "ListOffsetsApplicationEvent" here? it is in the "owner" section of the toStringBase too.

@lianetm lianetm Sep 15, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Similar for all other events that are explicitly including the class name and then calling the toStringBase)

toStringBase() +
", timestampsToSearch=" + timestampsToSearch +
", requireTimestamps=" + requireTimestamps +
'}';
}

}
9 changes: 8 additions & 1 deletion ...g/apache/kafka/clients/consumer/internals/events/NewTopicsMetadataUpdateRequestEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@
public class NewTopicsMetadataUpdateRequestEvent extends ApplicationEvent {

public NewTopicsMetadataUpdateRequestEvent() {
super(Type.METADATA_UPDATE);
super(Type.METADATA_UPDATE, NewTopicsMetadataUpdateRequestEvent.class.getSimpleName());
}

@Override
public String toString() {
return "NewTopicsMetadataUpdateRequestEvent{" +
toStringBase() +
'}';
}
}
7 changes: 5 additions & 2 deletions ...rc/main/java/org/apache/kafka/clients/consumer/internals/events/NoopApplicationEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,15 @@ public class NoopApplicationEvent extends ApplicationEvent {
public final String message;

public NoopApplicationEvent(final String message) {
super(Type.NOOP);
super(Type.NOOP, NoopApplicationEvent.class.getSimpleName());
this.message = message;
}

@Override
public String toString() {
return getClass() + "_" + this.message;
return "NoopApplicationEvent{" +
toStringBase() +
", message='" + message + '\'' +
'}';
}
}
11 changes: 5 additions & 6 deletions .../java/org/apache/kafka/clients/consumer/internals/events/OffsetFetchApplicationEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ public class OffsetFetchApplicationEvent extends CompletableApplicationEvent<Map
private final Set<TopicPartition> partitions;

public OffsetFetchApplicationEvent(final Set<TopicPartition> partitions) {
super(Type.FETCH_COMMITTED_OFFSET);
super(Type.FETCH_COMMITTED_OFFSET, OffsetFetchApplicationEvent.class.getSimpleName());
this.partitions = Collections.unmodifiableSet(partitions);
}

Expand Down Expand Up @@ -55,10 +55,9 @@ public int hashCode() {

@Override
public String toString() {
return getClass().getSimpleName() + "{" +
"partitions=" + partitions +
", future=" + future +
", type=" + type +
'}';
return "OffsetFetchApplicationEvent{" +
toStringBase() +
", partitions=" + partitions +
'}';
}
}
10 changes: 9 additions & 1 deletion ...rc/main/java/org/apache/kafka/clients/consumer/internals/events/PollApplicationEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,15 @@ public class PollApplicationEvent extends ApplicationEvent {
public final long pollTimeMs;

protected PollApplicationEvent(final long currentTimeMs) {
super(Type.POLL);
super(Type.POLL, PollApplicationEvent.class.getSimpleName());
this.pollTimeMs = currentTimeMs;
}

@Override
public String toString() {
return "PollApplicationEvent{" +
toStringBase() +
", pollTimeMs=" + pollTimeMs +
'}';
}
}
9 changes: 8 additions & 1 deletion ...va/org/apache/kafka/clients/consumer/internals/events/ResetPositionsApplicationEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@
public class ResetPositionsApplicationEvent extends CompletableApplicationEvent<Void> {

public ResetPositionsApplicationEvent() {
super(Type.RESET_POSITIONS);
super(Type.RESET_POSITIONS, ResetPositionsApplicationEvent.class.getSimpleName());
}

@Override
public String toString() {
return "ResetPositionsApplicationEvent{" +
toStringBase() +
'}';
}
}
9 changes: 8 additions & 1 deletion ...org/apache/kafka/clients/consumer/internals/events/ValidatePositionsApplicationEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@
public class ValidatePositionsApplicationEvent extends CompletableApplicationEvent<Void> {

public ValidatePositionsApplicationEvent() {
super(Type.VALIDATE_POSITIONS);
super(Type.VALIDATE_POSITIONS, ValidatePositionsApplicationEvent.class.getSimpleName());
}

@Override
public String toString() {
return "ValidatePositionsApplicationEvent{" +
toStringBase() +
'}';
}
}