Uploaded image for project: 'Apache NiFi'
  1. Apache NiFi
  2. NIFI-3719

Timezone error in 5-minute statistics processing time

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.2.0, 1.1.1
    • Fix Version/s: 1.3.0
    • Component/s: Core UI
    • Labels:
    • Environment:
      Windows 10 and Java 1.8.0_121.
      Also CentOS 7 with unknown Java version, but at least 1.7.

      Description

      With timezone set to Australia\Sydney (AEST, UCT+10), each processor shows Task / Time of 0 / 00:00:00.000.
      With timezone set to Australia\Adelaide (ACST, UCT+9:30), each processor shows Task / Time of 0 / 00:30:00.000.

        Issue Links

          Activity

          Hide
          mhopskip Michael Hopton added a comment -

          Nifi-3719 patch

          Show
          mhopskip Michael Hopton added a comment - Nifi-3719 patch
          Hide
          joewitt Joseph Witt added a comment -

          Thanks for your contribution.

          Have removed fix version until there is review traction/close to merge.

          Given the utility nature of this method I dont think we can simply force it to use UTC as it might cause behavior changes elsewhere. We should consider adding new method signatures that let the effective timezone be set explicitly, based on UTC, or whatever comes from the local system.

          Show
          joewitt Joseph Witt added a comment - Thanks for your contribution. Have removed fix version until there is review traction/close to merge. Given the utility nature of this method I dont think we can simply force it to use UTC as it might cause behavior changes elsewhere. We should consider adding new method signatures that let the effective timezone be set explicitly, based on UTC, or whatever comes from the local system.
          Hide
          mhopskip Michael Hopton added a comment -

          The methods are used to display durations, not times. Durations should not have a timezone. 20 seconds is 20 seconds irrespective of where in the world you are. However, Nifi thinks 20 seconds is 30:20 because of where I live.

          In Java 8, the Duration class from java.date should be used instead of using the Date class to represent a duration.
          The bug is caused by formatting durations as times.

          Show
          mhopskip Michael Hopton added a comment - The methods are used to display durations, not times. Durations should not have a timezone. 20 seconds is 20 seconds irrespective of where in the world you are. However, Nifi thinks 20 seconds is 30:20 because of where I live. In Java 8, the Duration class from java.date should be used instead of using the Date class to represent a duration. The bug is caused by formatting durations as times.
          Hide
          mcgilman Matt Gilman added a comment -

          Thanks for the patch!

          I believe the signatures of the methods in question would allow us to implement the formatting however we want to. If the bug is caused by formatting durations as times, should the proposed patch be updated to use the appropriate Duration class that Michael Hopton is suggesting?

          Show
          mcgilman Matt Gilman added a comment - Thanks for the patch! I believe the signatures of the methods in question would allow us to implement the formatting however we want to. If the bug is caused by formatting durations as times, should the proposed patch be updated to use the appropriate Duration class that Michael Hopton is suggesting?
          Hide
          mhopskip Michael Hopton added a comment -

          I think it would be ideal to migrate all dates, times and duration to the new Java 8 classes, but clearly that is a huge amount of effort. It also requires design to consider which class is correct for each date/time/duration. As such I suggest it might be a low priority.

          Show
          mhopskip Michael Hopton added a comment - I think it would be ideal to migrate all dates, times and duration to the new Java 8 classes, but clearly that is a huge amount of effort. It also requires design to consider which class is correct for each date/time/duration. As such I suggest it might be a low priority.
          Hide
          mcgilman Matt Gilman added a comment -

          Your correct in that it would be a large effort. Some of this effort would even affect some public APIs (specifically around the data model into/out of the REST API). Due to these additional hurdles, it may make sense to introduce these fixes iteratively as they surface. I'm happy to continue working with you on this specific issue if you'd like. Thanks again!

          Show
          mcgilman Matt Gilman added a comment - Your correct in that it would be a large effort. Some of this effort would even affect some public APIs (specifically around the data model into/out of the REST API). Due to these additional hurdles, it may make sense to introduce these fixes iteratively as they surface. I'm happy to continue working with you on this specific issue if you'd like. Thanks again!
          Hide
          mhopskip Michael Hopton added a comment -

          I've looked a bit more closely and java.time does not provide formatting for Duration, which seems to me to be a significant omission. As a consequence, changing to Duration ends up being completely unrelated to the bug reported and could be raised as a separate issue if there is a desire to refactor the code.

          I think we should stick with the patch provided to resolve the bug.

          Show
          mhopskip Michael Hopton added a comment - I've looked a bit more closely and java.time does not provide formatting for Duration, which seems to me to be a significant omission. As a consequence, changing to Duration ends up being completely unrelated to the bug reported and could be raised as a separate issue if there is a desire to refactor the code. I think we should stick with the patch provided to resolve the bug.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user mcgilman opened a pull request:

          https://github.com/apache/nifi/pull/1871

          NIFI-3719: Address timezone issue when formatting hours/minutes/seconds

          NIFI-3719:

          • Removing the usage of SimpleDateFormat when formatting hours/minutes/seconds as the current timezone could cause unintended results.

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

          $ git pull https://github.com/mcgilman/nifi NIFI-3719

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

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


          commit db77f7dd5356a38dae1b8b831a6e74fe992775ef
          Author: Matt Gilman <matt.c.gilman@gmail.com>
          Date: 2017-05-30T15:15:04Z

          NIFI-3719:

          • Removing the usage of SimpleDateFormat when formatting hours/minutes/seconds as the current timezone could cause unintended results.

          Show
          githubbot ASF GitHub Bot added a comment - GitHub user mcgilman opened a pull request: https://github.com/apache/nifi/pull/1871 NIFI-3719 : Address timezone issue when formatting hours/minutes/seconds NIFI-3719 : Removing the usage of SimpleDateFormat when formatting hours/minutes/seconds as the current timezone could cause unintended results. You can merge this pull request into a Git repository by running: $ git pull https://github.com/mcgilman/nifi NIFI-3719 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/nifi/pull/1871.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 #1871 commit db77f7dd5356a38dae1b8b831a6e74fe992775ef Author: Matt Gilman <matt.c.gilman@gmail.com> Date: 2017-05-30T15:15:04Z NIFI-3719 : Removing the usage of SimpleDateFormat when formatting hours/minutes/seconds as the current timezone could cause unintended results.
          Hide
          mcgilman Matt Gilman added a comment -

          I've offered up another patch that removes the usage of SimpleDateFormat in the formatting of hours/minutes/seconds.

          Show
          mcgilman Matt Gilman added a comment - I've offered up another patch that removes the usage of SimpleDateFormat in the formatting of hours/minutes/seconds.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 7bcccb10f4102daf73c99f7cc80b0036af0025b3 in nifi's branch refs/heads/master from Matt Gilman
          [ https://git-wip-us.apache.org/repos/asf?p=nifi.git;h=7bcccb1 ]

          NIFI-3719:

          • Removing the usage of SimpleDateFormat when formatting hours/minutes/seconds as the current timezone could cause unintended results.

          This closes #1871.

          Show
          jira-bot ASF subversion and git services added a comment - Commit 7bcccb10f4102daf73c99f7cc80b0036af0025b3 in nifi's branch refs/heads/master from Matt Gilman [ https://git-wip-us.apache.org/repos/asf?p=nifi.git;h=7bcccb1 ] NIFI-3719 : Removing the usage of SimpleDateFormat when formatting hours/minutes/seconds as the current timezone could cause unintended results. This closes #1871.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/nifi/pull/1871

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/nifi/pull/1871
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user markap14 commented on the issue:

          https://github.com/apache/nifi/pull/1871

          @mcgilman all looks good. I have merged to master.

          Show
          githubbot ASF GitHub Bot added a comment - Github user markap14 commented on the issue: https://github.com/apache/nifi/pull/1871 @mcgilman all looks good. I have merged to master.

            People

            • Assignee:
              mcgilman Matt Gilman
              Reporter:
              mhopskip Michael Hopton
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development