You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by kl0u <gi...@git.apache.org> on 2017/01/06 16:31:24 UTC
[GitHub] flink pull request #3075: [FLINK-5296] Expose the old AlignedWindowOperators...
GitHub user kl0u opened a pull request:
https://github.com/apache/flink/pull/3075
[FLINK-5296] Expose the old AlignedWindowOperators through special as\u2026
This PR allows the user to use the deprecated `AccumulatingProcessingTimeWindowOperator` and `AggregatingProcessingTimeWindowOperator` by specifying as a `WindowAssigner` when windowing, the `TumblingAlignedProcessingTimeWindows` or the `SlidingAlignedProcessingTimeWindows`.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kl0u/flink aligned_refactoring
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/3075.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #3075
----
commit 07322d829212f7ee7c7136c85d7a66aa54a417ed
Author: kl0u <kk...@gmail.com>
Date: 2016-12-20T12:51:45Z
[FLINK-5296] Expose the old AlignedWindowOperators through special assigners
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.
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3075: [FLINK-5296] Expose the old AlignedWindowOperators throug...
Posted by kl0u <gi...@git.apache.org>.
Github user kl0u commented on the issue:
https://github.com/apache/flink/pull/3075
@aljoscha Thanks for the review!
I will integrate the comments and ping you.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3075: [FLINK-5296] Expose the old AlignedWindowOperators throug...
Posted by kl0u <gi...@git.apache.org>.
Github user kl0u commented on the issue:
https://github.com/apache/flink/pull/3075
@aljoscha I integrated your comments.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3075: [FLINK-5296] Expose the old AlignedWindowOperators...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/3075#discussion_r95592206
--- Diff: flink-streaming-java/src/test/java/org/apache/flink/streaming/runtime/operators/windowing/TimeWindowTranslationTest.java ---
@@ -51,12 +53,48 @@
*/
public class TimeWindowTranslationTest {
+ @Test
--- End diff --
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.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3075: [FLINK-5296] Expose the old AlignedWindowOperators...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/3075#discussion_r95411735
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/windowing/assigners/BaseAlignedWindowAssigner.java ---
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.flink.streaming.api.windowing.assigners;
+
+import org.apache.flink.api.common.ExecutionConfig;
+import org.apache.flink.api.common.typeutils.TypeSerializer;
+import org.apache.flink.streaming.api.environment.StreamExecutionEnvironment;
+import org.apache.flink.streaming.api.windowing.triggers.Trigger;
+import org.apache.flink.streaming.api.windowing.windows.TimeWindow;
+
+import java.util.Collection;
+
+/**
+ * A base {@link WindowAssigner} used to instantiate one of the deprecated
+ * {@link org.apache.flink.streaming.runtime.operators.windowing.AccumulatingProcessingTimeWindowOperator
+ * AccumulatingProcessingTimeWindowOperator} and
+ * {@link org.apache.flink.streaming.runtime.operators.windowing.AggregatingProcessingTimeWindowOperator
+ * AggregatingProcessingTimeWindowOperator}.
+ * <p>
--- End diff --
Missing newline.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3075: [FLINK-5296] Expose the old AlignedWindowOperators...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/3075#discussion_r95594190
--- Diff: flink-streaming-java/src/test/java/org/apache/flink/streaming/runtime/operators/windowing/TimeWindowTranslationTest.java ---
@@ -51,12 +53,48 @@
*/
public class TimeWindowTranslationTest {
+ @Test
--- End diff --
True, then we can keep it but should update the method name and maybe give a javadoc that describes them.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3075: [FLINK-5296] Expose the old AlignedWindowOperators...
Posted by kl0u <gi...@git.apache.org>.
Github user kl0u closed the pull request at:
https://github.com/apache/flink/pull/3075
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3075: [FLINK-5296] Expose the old AlignedWindowOperators...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/3075#discussion_r95412067
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/windowing/assigners/TumblingAlignedProcessingTimeWindows.java ---
@@ -0,0 +1,53 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.streaming.api.windowing.assigners;
+
+import org.apache.flink.streaming.api.windowing.time.Time;
+
+/**
+ * A processing time tumbling {@link WindowAssigner window assigner} used to perform windowing using the
+ * {@link org.apache.flink.streaming.runtime.operators.windowing.AccumulatingProcessingTimeWindowOperator
+ * AccumulatingProcessingTimeWindowOperator} and the
+ * {@link org.apache.flink.streaming.runtime.operators.windowing.AggregatingProcessingTimeWindowOperator
+ * AggregatingProcessingTimeWindowOperator}.
+ * <p>
--- End diff --
Same comments as for the other assigner hold.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3075: [FLINK-5296] Expose the old AlignedWindowOperators throug...
Posted by kl0u <gi...@git.apache.org>.
Github user kl0u commented on the issue:
https://github.com/apache/flink/pull/3075
@aljoscha thanks for the review.
I changed the names of the tests and also put them in the right classes.
Let me know what you think.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3075: [FLINK-5296] Expose the old AlignedWindowOperators...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/3075#discussion_r95411761
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/windowing/assigners/SlidingAlignedProcessingTimeWindows.java ---
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.streaming.api.windowing.assigners;
+
+import org.apache.flink.streaming.api.windowing.time.Time;
+
+/**
+ * A processing time sliding {@link WindowAssigner window assigner} used to perform windowing using the
+ * {@link org.apache.flink.streaming.runtime.operators.windowing.AccumulatingProcessingTimeWindowOperator
+ * AccumulatingProcessingTimeWindowOperator} and the
+ * {@link org.apache.flink.streaming.runtime.operators.windowing.AggregatingProcessingTimeWindowOperator
+ * AggregatingProcessingTimeWindowOperator}.
+ * <p>
--- End diff --
Missing newline.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3075: [FLINK-5296] Expose the old AlignedWindowOperators throug...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/3075
Thanks for your work! \U0001f44d
Could you please close this PR? I merged it.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3075: [FLINK-5296] Expose the old AlignedWindowOperators...
Posted by kl0u <gi...@git.apache.org>.
Github user kl0u commented on a diff in the pull request:
https://github.com/apache/flink/pull/3075#discussion_r95593492
--- Diff: flink-streaming-scala/src/test/scala/org/apache/flink/streaming/api/scala/WindowTranslationTest.scala ---
@@ -48,6 +49,45 @@ import org.junit.Test
*/
class WindowTranslationTest {
+ @Test
--- End diff --
Same as comment as above.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3075: [FLINK-5296] Expose the old AlignedWindowOperators...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/3075#discussion_r95411773
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/windowing/assigners/SlidingAlignedProcessingTimeWindows.java ---
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.streaming.api.windowing.assigners;
+
+import org.apache.flink.streaming.api.windowing.time.Time;
+
+/**
+ * A processing time sliding {@link WindowAssigner window assigner} used to perform windowing using the
+ * {@link org.apache.flink.streaming.runtime.operators.windowing.AccumulatingProcessingTimeWindowOperator
+ * 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>
--- End diff --
Missing newline.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3075: [FLINK-5296] Expose the old AlignedWindowOperators...
Posted by kl0u <gi...@git.apache.org>.
Github user kl0u commented on a diff in the pull request:
https://github.com/apache/flink/pull/3075#discussion_r95593424
--- Diff: flink-streaming-java/src/test/java/org/apache/flink/streaming/runtime/operators/windowing/TimeWindowTranslationTest.java ---
@@ -51,12 +53,48 @@
*/
public class TimeWindowTranslationTest {
+ @Test
--- End diff --
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?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3075: [FLINK-5296] Expose the old AlignedWindowOperators throug...
Posted by kl0u <gi...@git.apache.org>.
Github user kl0u commented on the issue:
https://github.com/apache/flink/pull/3075
Thanks @aljoscha . Will do that.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3075: [FLINK-5296] Expose the old AlignedWindowOperators...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/3075#discussion_r95412039
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/windowing/assigners/SlidingAlignedProcessingTimeWindows.java ---
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.streaming.api.windowing.assigners;
+
+import org.apache.flink.streaming.api.windowing.time.Time;
+
+/**
+ * A processing time sliding {@link WindowAssigner window assigner} used to perform windowing using the
+ * {@link org.apache.flink.streaming.runtime.operators.windowing.AccumulatingProcessingTimeWindowOperator
+ * 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>
+ * Bare in mind that no rescaling and no backwards compatibility is supported.
--- End diff --
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)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3075: [FLINK-5296] Expose the old AlignedWindowOperators...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/3075#discussion_r95411689
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/datastream/WindowedStream.java ---
@@ -977,6 +1012,79 @@ private LegacyWindowOperatorType getLegacyWindowType(Function function) {
return LegacyWindowOperatorType.NONE;
}
+ private <R> SingleOutputStreamOperator<R> createFastTimeOperatorIfValid(
--- End diff --
This is simply a copy of the old code, right?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3075: [FLINK-5296] Expose the old AlignedWindowOperators...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/3075#discussion_r95592514
--- Diff: flink-streaming-scala/src/test/scala/org/apache/flink/streaming/api/scala/WindowTranslationTest.scala ---
@@ -48,6 +49,45 @@ import org.junit.Test
*/
class WindowTranslationTest {
+ @Test
--- End diff --
This is probably also a leftover from before I refactored the `WindowTranslationTest`?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---