pvary commented on PR #23555: URL: https://github.com/apache/flink/pull/23555#issuecomment-1777176669
> Apart from those comments, I'd like to do a second pass on the following: it looks like wiring in the new new committables metrics groups is adding quite a bit of "fat" w.r.t. external dependencies of various classes, e.g. `CommittableCollector` and the various serializers. I say they are redundant "fat" because ultimately only the end of the dependency chain, i.e. `CheckpointCommittableManagerImpl`, `SubtaskCommittableManager`, and `CommitRequestImpl` is doing meaningful logic on the metric group - all others earlier in the chain are just passing down the metric group and it looks rather polluted. This is rather evident in the test code changes, and isn't so nice. I tried before posting the PR, but were not able to come up with a better solution. After your review I did another stab on it: https://github.com/pvary/flink/tree/committable_context, https://github.com/pvary/flink/commit/7fce697e8ff586e80d810eed2c0cf48eb66c9f1b What I did here, is created a `CommittableContext` with `checkpointId`, `subtaskId`, `metricsGroup`, and propagated this context instead of the individual parameters. This causes a behavioural change - the `null` checkpointId has been changed to `Long.MAX_VALUE`, which I think is not acceptable. See: https://github.com/pvary/flink/commit/7fce697e8ff586e80d810eed2c0cf48eb66c9f1b Also the proposed change above only simplifies `CheckpointCommittableManagerImpl`, `CommitRequestImpl`, `SubtaskCommittableManager.java`, but does not help in `CommiterOperator` (uses/propagates only metricGroup), `CommittableCollector` (uses/propagates only metricGroup and subtaskId), `CommittableCollectorSerializer` (uses/propagates only metricGroup and subtaskId) Sadly, I do not see any better options ATM 😢 -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
