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

Fix incorrect check in allowedLateness() method. Make it a no-op for non-event time windows.

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.1.0, 1.1.1, 1.1.2, 1.1.3
    • Fix Version/s: 1.2.0, 1.3.0
    • Component/s: Streaming
    • Labels:
      None

      Description

      Related to FLINK-3714 and FLINK-4239

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user mindprince opened a pull request:

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

          FLINK-5247 Fix check to make sure that we throw error when allowed lateness is set for non event-time windows.

          Also, fix outdated documentation.

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

          $ git pull https://github.com/mindprince/flink FLINK-5247-allowed-lateness

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

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


          commit 10d523865e07df61e5a1c2b35e2201b498c46426
          Author: Rohit Agarwal <mindprince@gmail.com>
          Date: 2016-12-03T20:15:45Z

          FLINK-5247 Fix check to make sure that we throw error when allowed lateness is set for non event-time windows.

          Also, fix outdated documentation.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user mindprince opened a pull request: https://github.com/apache/flink/pull/2929 FLINK-5247 Fix check to make sure that we throw error when allowed lateness is set for non event-time windows. Also, fix outdated documentation. You can merge this pull request into a Git repository by running: $ git pull https://github.com/mindprince/flink FLINK-5247 -allowed-lateness Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/2929.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 #2929 commit 10d523865e07df61e5a1c2b35e2201b498c46426 Author: Rohit Agarwal <mindprince@gmail.com> Date: 2016-12-03T20:15:45Z FLINK-5247 Fix check to make sure that we throw error when allowed lateness is set for non event-time windows. Also, fix outdated documentation.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user mindprince commented on the issue:

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

          cc - @kl0u, @aljoscha for review

          Show
          githubbot ASF GitHub Bot added a comment - Github user mindprince commented on the issue: https://github.com/apache/flink/pull/2929 cc - @kl0u, @aljoscha for review
          Hide
          aljoscha Aljoscha Krettek added a comment -

          I'm not sure we actually want this. The only reason for the existence of the "stream time characteristic" (settable on StreamExecutionEnvironment) is to be able to quickly switch a pipeline between processing time and event time. (Whether that's helpful beyond simple demos/explorations is a different discussion)

          If we throw an exception instead of defaulting to zero internally we break this feature. Setting an allowed lateness other than zero for processing time windows is not per se wrong. It just cannot ever occur that we see elements so defaulting to zero internally is a runtime optimisation.

          Show
          aljoscha Aljoscha Krettek added a comment - I'm not sure we actually want this. The only reason for the existence of the "stream time characteristic" (settable on StreamExecutionEnvironment ) is to be able to quickly switch a pipeline between processing time and event time. (Whether that's helpful beyond simple demos/explorations is a different discussion) If we throw an exception instead of defaulting to zero internally we break this feature. Setting an allowed lateness other than zero for processing time windows is not per se wrong. It just cannot ever occur that we see elements so defaulting to zero internally is a runtime optimisation.
          Hide
          ragarwal Rohit Agarwal added a comment -

          The existing code already had provisions to throw exceptions when allowedLateness is set to non-zero value for processing time windows.

           			throw new IllegalArgumentException("Setting the allowed lateness is only valid for event-time windows.");
          

          I created this JIRA because that existing code was not working as intended: https://github.com/apache/flink/pull/2929/files

          Currently, if you set allowedLateness to non-zero value for processing-time windows, flink would update allowedLateness to that value. (This probably results in delays in window purging.)

          We can make allowedLateness method a no-op for processing-time windows, I didn't do that because looking at the existing code that didn't seem to be the intention.

          Show
          ragarwal Rohit Agarwal added a comment - The existing code already had provisions to throw exceptions when allowedLateness is set to non-zero value for processing time windows. throw new IllegalArgumentException( "Setting the allowed lateness is only valid for event-time windows." ); I created this JIRA because that existing code was not working as intended: https://github.com/apache/flink/pull/2929/files Currently, if you set allowedLateness to non-zero value for processing-time windows, flink would update allowedLateness to that value. (This probably results in delays in window purging.) We can make allowedLateness method a no-op for processing-time windows, I didn't do that because looking at the existing code that didn't seem to be the intention.
          Hide
          aljoscha Aljoscha Krettek added a comment -

          I see. Thanks for looking into this!

          I actually have a fix for this (ignore allowedLateness for processing time) buried in this PR: https://github.com/apache/flink/pull/2572/files#diff-408a499e1a35840c52e29b7ccab866b1R525 (You can look at WindowOperator.cleanupTime()).

          I think it would be better to ignore setting an allowed lateness for processing time. But you're right that the checks should be fixed.

          Show
          aljoscha Aljoscha Krettek added a comment - I see. Thanks for looking into this! I actually have a fix for this (ignore allowedLateness for processing time) buried in this PR: https://github.com/apache/flink/pull/2572/files#diff-408a499e1a35840c52e29b7ccab866b1R525 (You can look at WindowOperator.cleanupTime()). I think it would be better to ignore setting an allowed lateness for processing time. But you're right that the checks should be fixed.
          Hide
          ragarwal Rohit Agarwal added a comment -

          So, do you want me to update the PR to not throw the exception and make allowedLateness method a no-op for processing-time windows or is the current PR fine?

          Show
          ragarwal Rohit Agarwal added a comment - So, do you want me to update the PR to not throw the exception and make allowedLateness method a no-op for processing-time windows or is the current PR fine?
          Hide
          aljoscha Aljoscha Krettek added a comment -

          Changing the scope of this issue and then changing the PR would be good.

          Thanks for working on this, again!

          Show
          aljoscha Aljoscha Krettek added a comment - Changing the scope of this issue and then changing the PR would be good. Thanks for working on this, again!
          Hide
          aljoscha Aljoscha Krettek added a comment -

          Hi Rohit Agarwal, any updates on this?

          Show
          aljoscha Aljoscha Krettek added a comment - Hi Rohit Agarwal , any updates on this?
          Hide
          ragarwal Rohit Agarwal added a comment -

          Sorry, I am a bit busy these days. I will try to get this done during the weekend.

          Show
          ragarwal Rohit Agarwal added a comment - Sorry, I am a bit busy these days. I will try to get this done during the weekend.
          Hide
          ragarwal Rohit Agarwal added a comment -

          Hi Aljoscha Krettek,

          I updated the PR with what be discussed. Please take a look.

          Show
          ragarwal Rohit Agarwal added a comment - Hi Aljoscha Krettek , I updated the PR with what be discussed. Please take a look.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user mindprince commented on the issue:

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

          Ping @aljoscha

          Show
          githubbot ASF GitHub Bot added a comment - Github user mindprince commented on the issue: https://github.com/apache/flink/pull/2929 Ping @aljoscha
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user mindprince commented on the issue:

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

          Thanks for the review @kl0u

          I didn't do it because there was no logger defined in the file.

          Show
          githubbot ASF GitHub Bot added a comment - Github user mindprince commented on the issue: https://github.com/apache/flink/pull/2929 Thanks for the review @kl0u I didn't do it because there was no logger defined in the file.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user kl0u commented on the issue:

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

          Hi @mindprince . What do you mean "in the file"? The allowed lateness can always be specified in the program by the developer, right? So logging a message is always a valid approach.

          Show
          githubbot ASF GitHub Bot added a comment - Github user kl0u commented on the issue: https://github.com/apache/flink/pull/2929 Hi @mindprince . What do you mean "in the file"? The allowed lateness can always be specified in the program by the developer, right? So logging a message is always a valid approach.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user xhumanoid commented on the issue:

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

          I think @mindprince mean: at now logger not defined in class file

          this classes without
          private static final Logger LOG

          Show
          githubbot ASF GitHub Bot added a comment - Github user xhumanoid commented on the issue: https://github.com/apache/flink/pull/2929 I think @mindprince mean: at now logger not defined in class file this classes without private static final Logger LOG
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on the issue:

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

          Thank you for fixing this.
          I think it is good as it is, will merge this for Flink-1.2 and master...

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/2929 Thank you for fixing this. I think it is good as it is, will merge this for Flink-1.2 and master...
          Hide
          StephanEwen Stephan Ewen added a comment -

          Fixed in

          • 1.2.0 via 4697b97a0101cf04b43c4a6e4887adba10b4a69a
          • 1.3.0 via 87af84194911eb1e0c3b3a894bb3f04b628fbf11
          Show
          StephanEwen Stephan Ewen added a comment - Fixed in 1.2.0 via 4697b97a0101cf04b43c4a6e4887adba10b4a69a 1.3.0 via 87af84194911eb1e0c3b3a894bb3f04b628fbf11
          Show
          githubbot ASF GitHub Bot added a comment - Github user mindprince commented on the issue: https://github.com/apache/flink/pull/2929 Merged in https://github.com/apache/flink/commit/87af84194911eb1e0c3b3a894bb3f04b628fbf11 and https://github.com/apache/flink/commit/4697b97a0101cf04b43c4a6e4887adba10b4a69a
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user mindprince closed the pull request at:

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user mindprince closed the pull request at: https://github.com/apache/flink/pull/2929

            People

            • Assignee:
              Unassigned
              Reporter:
              ragarwal Rohit Agarwal
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development