Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/570#issuecomment-100996059
I had a look at the pull request and I like very much what it tries to do.
The problem right now is that I can hardly say without investing a lot of
time whether this is in good shape to merge. This pull request does a at least
two very big things at the same time:
- Move iteration synchronization to the JobManager
- Unify aggregators and accumulators into one.
With all the example / testcase adjustments, this becomes a lot to review.
The description of the pull request also does not make it easy, since many
questions and decisions that arise are not explained:
- What interface do the unified aggregators/accumulators follow: The
aggregators, or the accumulators.
- How is the blocking superstep synchronization currently done. With actor
ask?
- How is the aggregator/accumulator unification achieved, when aggregators
are created per superstep, and accumulators once?
This is a lot for a very delicate and critical mechanism. I think if we
want to merge this, we would need more details on how things were changed (what
is the concept behind the changed, not just what are the code diffs).
We may need to break it into multiple self-contained changes that we can
individually review and merge, to make sure that it gets properly checked and
will work robustly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
