Copilot commented on code in PR #4350:
URL: https://github.com/apache/flink-cdc/pull/4350#discussion_r2999863737
##########
flink-cdc-common/src/main/java/org/apache/flink/cdc/common/event/visitor/SchemaChangeEventVisitor.java:
##########
@@ -87,4 +87,53 @@ public static <T, E extends Throwable> T visit(
"Unknown schema change event type " + event.getType());
}
}
+
+ public static <E extends Throwable> void voidVisit(
+ SchemaChangeEvent event,
+ VoidVisitorHandler<AddColumnEvent, E> addColumnVisitor,
+ VoidVisitorHandler<AlterColumnTypeEvent, E>
alterColumnTypeEventVisitor,
+ VoidVisitorHandler<CreateTableEvent, E> createTableEventVisitor,
+ VoidVisitorHandler<DropColumnEvent, E> dropColumnEventVisitor,
+ VoidVisitorHandler<DropTableEvent, E> dropTableEventVisitor,
+ VoidVisitorHandler<RenameColumnEvent, E> renameColumnEventVisitor,
+ VoidVisitorHandler<TruncateTableEvent, E>
truncateTableEventVisitor,
+ VoidVisitorHandler<AlterTableCommentEvent, E>
alterTableCommentEventHandler)
+ throws E {
Review Comment:
`visit(...)` and `voidVisit(...)` duplicate the same long `instanceof`
dispatch chain. This increases the chance of future schema change event
additions being implemented in one method but forgotten in the other. Consider
factoring the dispatch into a shared private method (or implementing
`voidVisit` by delegating to `visit` with a `Void` return) so new event types
only need to be added in one place.
```suggestion
throws E {
dispatchVoid(
event,
addColumnVisitor,
alterColumnTypeEventVisitor,
createTableEventVisitor,
dropColumnEventVisitor,
dropTableEventVisitor,
renameColumnEventVisitor,
truncateTableEventVisitor,
alterTableCommentEventHandler);
}
private static <E extends Throwable> void dispatchVoid(
SchemaChangeEvent event,
VoidVisitorHandler<AddColumnEvent, E> addColumnVisitor,
VoidVisitorHandler<AlterColumnTypeEvent, E>
alterColumnTypeEventVisitor,
VoidVisitorHandler<CreateTableEvent, E> createTableEventVisitor,
VoidVisitorHandler<DropColumnEvent, E> dropColumnEventVisitor,
VoidVisitorHandler<DropTableEvent, E> dropTableEventVisitor,
VoidVisitorHandler<RenameColumnEvent, E>
renameColumnEventVisitor,
VoidVisitorHandler<TruncateTableEvent, E>
truncateTableEventVisitor,
VoidVisitorHandler<AlterTableCommentEvent, E>
alterTableCommentEventHandler)
throws E {
```
##########
flink-cdc-common/src/main/java/org/apache/flink/cdc/common/event/visitor/SchemaChangeEventVisitor.java:
##########
@@ -33,14 +33,14 @@
public class SchemaChangeEventVisitor {
public static <T, E extends Throwable> T visit(
SchemaChangeEvent event,
- AddColumnEventVisitor<T, E> addColumnVisitor,
- AlterColumnTypeEventVisitor<T, E> alterColumnTypeEventVisitor,
- CreateTableEventVisitor<T, E> createTableEventVisitor,
- DropColumnEventVisitor<T, E> dropColumnEventVisitor,
- DropTableEventVisitor<T, E> dropTableEventVisitor,
- RenameColumnEventVisitor<T, E> renameColumnEventVisitor,
- TruncateTableEventVisitor<T, E> truncateTableEventVisitor,
- AlterTableCommentEventVisitor<T, E> alterTableCommentEventVisitor)
+ VisitorHandler<AddColumnEvent, T, E> addColumnVisitor,
+ VisitorHandler<AlterColumnTypeEvent, T, E>
alterColumnTypeEventVisitor,
+ VisitorHandler<CreateTableEvent, T, E> createTableEventVisitor,
+ VisitorHandler<DropColumnEvent, T, E> dropColumnEventVisitor,
+ VisitorHandler<DropTableEvent, T, E> dropTableEventVisitor,
+ VisitorHandler<RenameColumnEvent, T, E> renameColumnEventVisitor,
+ VisitorHandler<TruncateTableEvent, T, E> truncateTableEventVisitor,
+ VisitorHandler<AlterTableCommentEvent, T, E>
alterTableCommentEventVisitor)
throws E {
if (event instanceof AddColumnEvent) {
if (addColumnVisitor == null) {
Review Comment:
In `visit(...)`, the null-handler behavior has changed from fail-fast to
silently returning `null` (e.g., when `addColumnVisitor` is null). This can
mask programming errors and can propagate unexpected nulls to callers (e.g.,
`SchemaChangeEventSerializer.copy(...)` expects a non-null return). Consider
restoring the previous behavior (throwing an `IllegalArgumentException`) or
explicitly documenting and handling the `null` result everywhere.
##########
flink-cdc-common/src/main/java/org/apache/flink/cdc/common/event/visitor/SchemaChangeEventVisitor.java:
##########
@@ -87,4 +87,53 @@ public static <T, E extends Throwable> T visit(
"Unknown schema change event type " + event.getType());
}
}
+
+ public static <E extends Throwable> void voidVisit(
+ SchemaChangeEvent event,
+ VoidVisitorHandler<AddColumnEvent, E> addColumnVisitor,
+ VoidVisitorHandler<AlterColumnTypeEvent, E>
alterColumnTypeEventVisitor,
+ VoidVisitorHandler<CreateTableEvent, E> createTableEventVisitor,
+ VoidVisitorHandler<DropColumnEvent, E> dropColumnEventVisitor,
+ VoidVisitorHandler<DropTableEvent, E> dropTableEventVisitor,
+ VoidVisitorHandler<RenameColumnEvent, E> renameColumnEventVisitor,
+ VoidVisitorHandler<TruncateTableEvent, E>
truncateTableEventVisitor,
+ VoidVisitorHandler<AlterTableCommentEvent, E>
alterTableCommentEventHandler)
+ throws E {
+ if (event instanceof AddColumnEvent) {
+ if (addColumnVisitor != null) {
+ addColumnVisitor.visit((AddColumnEvent) event);
+ }
+ } else if (event instanceof AlterColumnTypeEvent) {
+ if (alterColumnTypeEventVisitor != null) {
+ alterColumnTypeEventVisitor.visit((AlterColumnTypeEvent)
event);
+ }
+ } else if (event instanceof CreateTableEvent) {
+ if (createTableEventVisitor != null) {
+ createTableEventVisitor.visit((CreateTableEvent) event);
+ }
+ } else if (event instanceof DropColumnEvent) {
+ if (dropColumnEventVisitor != null) {
+ dropColumnEventVisitor.visit((DropColumnEvent) event);
+ }
+ } else if (event instanceof DropTableEvent) {
+ if (dropTableEventVisitor != null) {
+ dropTableEventVisitor.visit((DropTableEvent) event);
+ }
+ } else if (event instanceof RenameColumnEvent) {
+ if (renameColumnEventVisitor != null) {
+ renameColumnEventVisitor.visit((RenameColumnEvent) event);
+ }
+ } else if (event instanceof TruncateTableEvent) {
+ if (truncateTableEventVisitor != null) {
+ truncateTableEventVisitor.visit((TruncateTableEvent) event);
+ }
+ } else if (event instanceof AlterTableCommentEvent) {
+ if (alterTableCommentEventHandler != null) {
+ alterTableCommentEventHandler.visit((AlterTableCommentEvent)
event);
+ }
Review Comment:
`voidVisit(...)` currently treats a null handler as a no-op (the event is
effectively dropped). For schema change processing (serialization / metadata
application), silently skipping an event is dangerous and makes errors hard to
diagnose. Please consider aligning the behavior with `visit(...)` by failing
fast when the matching handler is null (or at least throwing for the matched
event type), rather than quietly ignoring it.
```suggestion
if (addColumnVisitor == null) {
throw new IllegalArgumentException(
"No void visitor handler provided for
AddColumnEvent");
}
addColumnVisitor.visit((AddColumnEvent) event);
} else if (event instanceof AlterColumnTypeEvent) {
if (alterColumnTypeEventVisitor == null) {
throw new IllegalArgumentException(
"No void visitor handler provided for
AlterColumnTypeEvent");
}
alterColumnTypeEventVisitor.visit((AlterColumnTypeEvent) event);
} else if (event instanceof CreateTableEvent) {
if (createTableEventVisitor == null) {
throw new IllegalArgumentException(
"No void visitor handler provided for
CreateTableEvent");
}
createTableEventVisitor.visit((CreateTableEvent) event);
} else if (event instanceof DropColumnEvent) {
if (dropColumnEventVisitor == null) {
throw new IllegalArgumentException(
"No void visitor handler provided for
DropColumnEvent");
}
dropColumnEventVisitor.visit((DropColumnEvent) event);
} else if (event instanceof DropTableEvent) {
if (dropTableEventVisitor == null) {
throw new IllegalArgumentException(
"No void visitor handler provided for
DropTableEvent");
}
dropTableEventVisitor.visit((DropTableEvent) event);
} else if (event instanceof RenameColumnEvent) {
if (renameColumnEventVisitor == null) {
throw new IllegalArgumentException(
"No void visitor handler provided for
RenameColumnEvent");
}
renameColumnEventVisitor.visit((RenameColumnEvent) event);
} else if (event instanceof TruncateTableEvent) {
if (truncateTableEventVisitor == null) {
throw new IllegalArgumentException(
"No void visitor handler provided for
TruncateTableEvent");
}
truncateTableEventVisitor.visit((TruncateTableEvent) event);
} else if (event instanceof AlterTableCommentEvent) {
if (alterTableCommentEventHandler == null) {
throw new IllegalArgumentException(
"No void visitor handler provided for
AlterTableCommentEvent");
}
alterTableCommentEventHandler.visit((AlterTableCommentEvent)
event);
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
