dawidwys commented on a change in pull request #11692: URL: https://github.com/apache/flink/pull/11692#discussion_r411147490
########## File path: flink-table/flink-table-api-java-bridge/src/main/java/org/apache/flink/table/connector/source/abilities/PeriodicWatermarkAssignerProvider.java ########## @@ -28,19 +28,14 @@ * generating watermarks in {@link ScanTableSource}. */ @PublicEvolving -public final class PeriodicWatermarkAssignerProvider extends SupportsWatermarkPushDown.WatermarkProvider { +public interface PeriodicWatermarkAssignerProvider extends SupportsWatermarkPushDown.WatermarkProvider { - private final AssignerWithPeriodicWatermarks<RowData> periodicAssigner; - - private PeriodicWatermarkAssignerProvider(AssignerWithPeriodicWatermarks<RowData> periodicAssigner) { - this.periodicAssigner = periodicAssigner; - } - - public static PeriodicWatermarkAssignerProvider of(AssignerWithPeriodicWatermarks<RowData> periodicAssigner) { - return new PeriodicWatermarkAssignerProvider(periodicAssigner); + /** + * Helper method for creating a static provider. + */ + static PeriodicWatermarkAssignerProvider of(AssignerWithPeriodicWatermarks<RowData> periodicAssigner) { Review comment: This method does not make much sense, does it? Planner will always need to create a Provider that implements at least two of the interfaces e.g. `PeriodicWatermarkAssignerProvider` and FLIP-27 `WatermarkProvider`? Moreover we probably want to have some supplier interface here. We don't necessarily need to instantiate the `AssignerWithPeriodicWatermarks` if the source works with the interface from FLIP-27. Can we just remove this method? I really can't see any benefit of it whatsoever. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org