Github user xccui commented on the issue:
https://github.com/apache/flink/pull/4530
I totally understand the choice, @fhueske ð
Thanks for the refactoring.
---
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
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/4530
You're right @xccui, this is a trade off.
I thought about this again and agree with @aljoscha that it would be better
to avoid the additional method call. The `processWatermark()` method is
Github user xccui commented on the issue:
https://github.com/apache/flink/pull/4530
Thanks for the comment, @aljoscha. IMO, making the `timeServiceManager`
protected indeed will minimise the impact on `AbstractStreamOperator`, while
that may introduce duplicated codes in the
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/4530
I think the approach is OK, though I personally would have preferred to
make the instance variable protected and override `processWatermark()` to
minimise impact on `AbstractStreamOperator`, which
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/4530
I see, didn't consider the private timeServiceManager. Maybe this is the
best approach then.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user xccui commented on the issue:
https://github.com/apache/flink/pull/4530
Thanks for the comments @fhueske. I will pay more attention to the coding
style.
Actually, there are many ways to implement this feature. At first, I planed
to override the
Github user wuchong commented on the issue:
https://github.com/apache/flink/pull/4530
I'm fine with the changes. +1 to merge
---
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
Github user xccui commented on the issue:
https://github.com/apache/flink/pull/4530
@fhueske Yes, the plural is better. I should have noticed that before.
This PR is updated with the new package name and an extra delay parameter
added to the co-operator.
---
If your project is
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/4530
Good proposal @xccui! I'd prefer the plural:
`org.apache.flink.table.runtime.operators`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user xccui commented on the issue:
https://github.com/apache/flink/pull/4530
Thanks for the suggestion, @aljoscha. Do you think it's appropriate to add
a new package `org.apache.flink.table.runtime.operator`?
---
If your project is set up for it, you can reply to this email
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/4530
I think the new operators can go into the Table API packages. They are not
usable from the public `DataStream API`.
---
If your project is set up for it, you can reply to this email and have your
11 matches
Mail list logo
