Uploaded image for project: 'Flink'
  1. Flink
  2. FLINK-5532

Make the marker WindowAssigners for the fast aligned windows non-extendable.

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.2.0
    • Fix Version/s: 1.2.0, 1.3.0
    • Component/s: DataStream API
    • Labels:
      None

      Description

      This issue refers to the SlidingAlignedProcessingTimeWindows and TumblingAlignedProcessingTimeWindows and proposes to:
      (1) make the classes final
      (2) use getClass() for the check, not instanceof()

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user kl0u opened a pull request:

          https://github.com/apache/flink/pull/3180

          FLINK-5532 Make window assigners for aligned window operators non-extendable

          Makes the TumblingAlignedProcessingTimeWindows and the
          SlidingAlignedProcessingTimeWindows final so that users cannot
          extend them.

          R @aljoscha @StephanEwen

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/kl0u/flink non-extendable-aligned

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/flink/pull/3180.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 #3180


          commit 82bb3519e5bb024774795adcded0a51d9bc2fa4c
          Author: kl0u <kkloudas@gmail.com>
          Date: 2017-01-20T13:31:48Z

          FLINK-5532 Make window assigners for aligned window ops non-extendable

          Makes the TumblingAlignedProcessingTimeWindows and the
          SlidingAlignedProcessingTimeWindows final so that users cannot
          extend them.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user kl0u opened a pull request: https://github.com/apache/flink/pull/3180 FLINK-5532 Make window assigners for aligned window operators non-extendable Makes the TumblingAlignedProcessingTimeWindows and the SlidingAlignedProcessingTimeWindows final so that users cannot extend them. R @aljoscha @StephanEwen You can merge this pull request into a Git repository by running: $ git pull https://github.com/kl0u/flink non-extendable-aligned Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3180.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 #3180 commit 82bb3519e5bb024774795adcded0a51d9bc2fa4c Author: kl0u <kkloudas@gmail.com> Date: 2017-01-20T13:31:48Z FLINK-5532 Make window assigners for aligned window ops non-extendable Makes the TumblingAlignedProcessingTimeWindows and the SlidingAlignedProcessingTimeWindows final so that users cannot extend them.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fhueske commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3180#discussion_r97283576

          — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/windowing/assigners/SlidingAlignedProcessingTimeWindows.java —
          @@ -35,7 +35,7 @@

          • <p>
          • <b>WARNING:</b> Bear in mind that no rescaling and no backwards compatibility is supported.
          • */
            -public class SlidingAlignedProcessingTimeWindows extends BaseAlignedWindowAssigner {
            +public final class SlidingAlignedProcessingTimeWindows extends BaseAlignedWindowAssigner {
              • End diff –

          This class does not have an annotation.
          Should we add `@PublicEvolving`?

          Same is true for the `TumblingAlignedProcessingTImeWindows`.

          Show
          githubbot ASF GitHub Bot added a comment - Github user fhueske commented on a diff in the pull request: https://github.com/apache/flink/pull/3180#discussion_r97283576 — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/windowing/assigners/SlidingAlignedProcessingTimeWindows.java — @@ -35,7 +35,7 @@ <p> <b>WARNING:</b> Bear in mind that no rescaling and no backwards compatibility is supported. */ -public class SlidingAlignedProcessingTimeWindows extends BaseAlignedWindowAssigner { +public final class SlidingAlignedProcessingTimeWindows extends BaseAlignedWindowAssigner { End diff – This class does not have an annotation. Should we add `@PublicEvolving`? Same is true for the `TumblingAlignedProcessingTImeWindows`.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user kl0u commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3180#discussion_r97283933

          — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/windowing/assigners/SlidingAlignedProcessingTimeWindows.java —
          @@ -35,7 +35,7 @@

          • <p>
          • <b>WARNING:</b> Bear in mind that no rescaling and no backwards compatibility is supported.
          • */
            -public class SlidingAlignedProcessingTimeWindows extends BaseAlignedWindowAssigner {
            +public final class SlidingAlignedProcessingTimeWindows extends BaseAlignedWindowAssigner {
              • End diff –

          You are right @fhueske . I will make them `@deprecated` and `@PublicEvolving`.

          Show
          githubbot ASF GitHub Bot added a comment - Github user kl0u commented on a diff in the pull request: https://github.com/apache/flink/pull/3180#discussion_r97283933 — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/windowing/assigners/SlidingAlignedProcessingTimeWindows.java — @@ -35,7 +35,7 @@ <p> <b>WARNING:</b> Bear in mind that no rescaling and no backwards compatibility is supported. */ -public class SlidingAlignedProcessingTimeWindows extends BaseAlignedWindowAssigner { +public final class SlidingAlignedProcessingTimeWindows extends BaseAlignedWindowAssigner { End diff – You are right @fhueske . I will make them `@deprecated` and `@PublicEvolving`.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3180#discussion_r97283937

          — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/windowing/assigners/SlidingAlignedProcessingTimeWindows.java —
          @@ -35,7 +35,7 @@

          • <p>
          • <b>WARNING:</b> Bear in mind that no rescaling and no backwards compatibility is supported.
          • */
            -public class SlidingAlignedProcessingTimeWindows extends BaseAlignedWindowAssigner {
            +public final class SlidingAlignedProcessingTimeWindows extends BaseAlignedWindowAssigner {
              • End diff –

          Yes, they should be `@PublicEvolving`.
          Will add that when merging...

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3180#discussion_r97283937 — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/windowing/assigners/SlidingAlignedProcessingTimeWindows.java — @@ -35,7 +35,7 @@ <p> <b>WARNING:</b> Bear in mind that no rescaling and no backwards compatibility is supported. */ -public class SlidingAlignedProcessingTimeWindows extends BaseAlignedWindowAssigner { +public final class SlidingAlignedProcessingTimeWindows extends BaseAlignedWindowAssigner { End diff – Yes, they should be `@PublicEvolving`. Will add that when merging...
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on the issue:

          https://github.com/apache/flink/pull/3180

          +1 to merge this, modulo Fabian's comment...

          merging this...

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3180 +1 to merge this, modulo Fabian's comment... merging this...
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user kl0u commented on the issue:

          https://github.com/apache/flink/pull/3180

          Thanks @fhueske and @StephanEwen !

          Show
          githubbot ASF GitHub Bot added a comment - Github user kl0u commented on the issue: https://github.com/apache/flink/pull/3180 Thanks @fhueske and @StephanEwen !
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/flink/pull/3180

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/3180
          Hide
          StephanEwen Stephan Ewen added a comment -

          Fixed in

          • 1.2.0 via c291fe6c5e90ca7c6f11d2c93a4a3d370d5f8a8c
          • 1.3.0 via 509ec7c333336e35aa46bf80955d3a6502b6e506
          Show
          StephanEwen Stephan Ewen added a comment - Fixed in 1.2.0 via c291fe6c5e90ca7c6f11d2c93a4a3d370d5f8a8c 1.3.0 via 509ec7c333336e35aa46bf80955d3a6502b6e506

            People

            • Assignee:
              kkl0u Kostas Kloudas
              Reporter:
              kkl0u Kostas Kloudas
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development