echauchot commented on PR #22124: URL: https://github.com/apache/flink/pull/22124#issuecomment-1489898071
> @echauchot thanks for opening the PR. > > I think I agree with @mas-chen that this default implementation doesn't seem like a good idea. > > > the question can be reduced to "'what is the most common pattern among sources?" > > IMHO, in Flink there isn't a dominant common pattern, hence leaving the method `abstract` to enforce implementations to implement as needed. Streaming sources like Kafka / Pulsar / Kinesis (arguably the most important ones) all use split pushes. Batch sources on the other hand request splits explicitly. > > A few other comments on why it would be a -1 from me: > > * Having this default implementation means that implementations are opened up to call `super.onSplitFinished(...)`, which IMO can confuse implementors as they _shouldn't_ be calling the base implementation. > * The default implementation doesn't do any split state cleanup - that seems wrong as its making a strong assumption that there usually is no state to cleanup. > > All in all, I think the fact that it is an `abstract` method speaks a lot for itself - i.e. implementations can vary case by case and that there is no default implementation that is commonly shared / extractable across all cases. @tzulitai thanks for your arguments. Fair enough, closing the PR and the ticket -- 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]
