[FLINK-5296] Expose the old AlignedWindowOperators through special as…#3075
[FLINK-5296] Expose the old AlignedWindowOperators through special as…#3075kl0u wants to merge 2 commits intoapache:masterfrom
Conversation
aljoscha
left a comment
aljoscha
left a comment
There was a problem hiding this comment.
I like these changes! 👍 I had some comments about Javadocs.
| return LegacyWindowOperatorType.NONE; | ||
| } | ||
|
|
||
| private <R> SingleOutputStreamOperator<R> createFastTimeOperatorIfValid( |
There was a problem hiding this comment.
This is simply a copy of the old code, right?
| * AccumulatingProcessingTimeWindowOperator} and | ||
| * {@link org.apache.flink.streaming.runtime.operators.windowing.AggregatingProcessingTimeWindowOperator | ||
| * AggregatingProcessingTimeWindowOperator}. | ||
| * <p> |
| * AccumulatingProcessingTimeWindowOperator} and the | ||
| * {@link org.apache.flink.streaming.runtime.operators.windowing.AggregatingProcessingTimeWindowOperator | ||
| * AggregatingProcessingTimeWindowOperator}. | ||
| * <p> |
| * With this assigner, the {@code trigger} used is a | ||
| * {@link org.apache.flink.streaming.api.windowing.triggers.ProcessingTimeTrigger | ||
| * ProcessingTimeTrigger} and no {@code evictor} can be specified. | ||
| * <p> |
| * {@link org.apache.flink.streaming.api.windowing.triggers.ProcessingTimeTrigger | ||
| * ProcessingTimeTrigger} and no {@code evictor} can be specified. | ||
| * <p> | ||
| * Bare in mind that no rescaling and no backwards compatibility is supported. |
There was a problem hiding this comment.
I think we should have a bigger notice here, possibly with <b> and WARNING.
Also, I think it should be "bear in mind". (https://www.quora.com/Which-is-correct-bare-in-mind-or-bear-in-mind)
| * AccumulatingProcessingTimeWindowOperator} and the | ||
| * {@link org.apache.flink.streaming.runtime.operators.windowing.AggregatingProcessingTimeWindowOperator | ||
| * AggregatingProcessingTimeWindowOperator}. | ||
| * <p> |
There was a problem hiding this comment.
Same comments as for the other assigner hold.
|
@aljoscha Thanks for the review! |
kl0u
force-pushed
the
aligned_refactoring
branch
from
07322d8 to
6e9b912
Compare
|
@aljoscha I integrated your comments. |
…signers The user can use the deprecated AccumulatingProcessingTimeWindowOperator and AggregatingProcessingTimeWindowOperator by using the TumblingAlignedProcessingTimeWindows and the SlidingAlignedProcessingTimeWindows introduced by this commit. These operators are neither backwards compatibility nor rescalable.
kl0u
force-pushed
the
aligned_refactoring
branch
from
6e9b912 to
5d461d8
Compare
| */ | ||
| public class TimeWindowTranslationTest { | ||
|
|
||
| @Test |
There was a problem hiding this comment.
I think this should be covered by the other tests that now exist in this test suite? You probably still have this because these are very recent additions.
There was a problem hiding this comment.
Well this tests that now even for processing time, the timeWindow() leads to a WindowOperator being instantiated, rather than an aligned one. I can explicitly
add the TimeCharacteristic. What do you think?
There was a problem hiding this comment.
True, then we can keep it but should update the method name and maybe give a javadoc that describes them.
| */ | ||
| class WindowTranslationTest { | ||
|
|
||
| @Test |
There was a problem hiding this comment.
This is probably also a leftover from before I refactored the WindowTranslationTest?
There was a problem hiding this comment.
Same comment as above.
|
@aljoscha thanks for the review. |
|
Thanks for your work! 👍 Could you please close this PR? I merged it. |
|
Thanks @aljoscha . Will do that. |
3 participants
This PR allows the user to use the deprecated
AccumulatingProcessingTimeWindowOperatorandAggregatingProcessingTimeWindowOperatorby specifying as aWindowAssignerwhen windowing, theTumblingAlignedProcessingTimeWindowsor theSlidingAlignedProcessingTimeWindows.R @aljoscha