Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.6.0
    • Fix Version/s: 1.7.0
    • Component/s: None
    • Labels:
      None
    • Release Note:
      Fix year rolling issues with RFC3164 syslog data.

      Description

      date.withYear(year+1) can lead to incorrect date calculations .. for example if the date is Feb 29th. need to use date.plusYears(1) instead.

      1. FLUME-2889.3.patch
        7 kB
        Roshan Naik
      2. FLUME-2889.patch
        2 kB
        Roshan Naik
      3. FLUME-2889-2.patch
        5 kB
        Tristan Stevens
      4. FLUME-2889-4.patch
        9 kB
        Tristan Stevens

        Activity

        Hide
        roshan_naik Roshan Naik added a comment -

        Uploading patch... minor 1 liner fixes in a few places

        Show
        roshan_naik Roshan Naik added a comment - Uploading patch... minor 1 liner fixes in a few places
        Hide
        roshan_naik Roshan Naik added a comment -

        minor patch ... ready for review

        Show
        roshan_naik Roshan Naik added a comment - minor patch ... ready for review
        Hide
        hshreedharan Hari Shreedharan added a comment -

        +1. LGTM.

        Roshan Naik Please go ahead and commit it, else I will commit it today/tomorrow

        Show
        hshreedharan Hari Shreedharan added a comment - +1. LGTM. Roshan Naik Please go ahead and commit it, else I will commit it today/tomorrow
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit fe24607f42e2684128fbacdd73fa7489966aecc5 in flume's branch refs/heads/flume-1.7 from Roshan Naik
        [ https://git-wip-us.apache.org/repos/asf?p=flume.git;h=fe24607 ]

        FLUME-2889. Fixes to DateTime computations

        (Roshan Naik via Hari Shreedharan)

        Show
        jira-bot ASF subversion and git services added a comment - Commit fe24607f42e2684128fbacdd73fa7489966aecc5 in flume's branch refs/heads/flume-1.7 from Roshan Naik [ https://git-wip-us.apache.org/repos/asf?p=flume.git;h=fe24607 ] FLUME-2889 . Fixes to DateTime computations (Roshan Naik via Hari Shreedharan)
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit ffb52b9e656df51e5e6881cfc8ed851a89cdc2f1 in flume's branch refs/heads/trunk from Roshan Naik
        [ https://git-wip-us.apache.org/repos/asf?p=flume.git;h=ffb52b9 ]

        FLUME-2889. Fixes to DateTime computations

        (Roshan Naik via Hari Shreedharan)

        Show
        jira-bot ASF subversion and git services added a comment - Commit ffb52b9e656df51e5e6881cfc8ed851a89cdc2f1 in flume's branch refs/heads/trunk from Roshan Naik [ https://git-wip-us.apache.org/repos/asf?p=flume.git;h=ffb52b9 ] FLUME-2889 . Fixes to DateTime computations (Roshan Naik via Hari Shreedharan)
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Flume-trunk-hbase-1 #151 (See https://builds.apache.org/job/Flume-trunk-hbase-1/151/)
        FLUME-2889. Fixes to DateTime computations (roshan: http://git-wip-us.apache.org/repos/asf/flume/repo?p=flume.git&a=commit&h=ffb52b9e656df51e5e6881cfc8ed851a89cdc2f1)

        • flume-ng-core/src/test/java/org/apache/flume/serialization/SyslogAvroEventSerializer.java
        • flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Flume-trunk-hbase-1 #151 (See https://builds.apache.org/job/Flume-trunk-hbase-1/151/ ) FLUME-2889 . Fixes to DateTime computations (roshan: http://git-wip-us.apache.org/repos/asf/flume/repo?p=flume.git&a=commit&h=ffb52b9e656df51e5e6881cfc8ed851a89cdc2f1 ) flume-ng-core/src/test/java/org/apache/flume/serialization/SyslogAvroEventSerializer.java flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
        Hide
        tmgstev Tristan Stevens added a comment -

        Whoah, hold on a second. The default year on Jodatime is 2000, so fixed = date.minusYears(1); will set the year to 1999.

        I think what we need here is fixed=fixed.minusYears(1) (and the same for the plusYears).

        Show
        tmgstev Tristan Stevens added a comment - Whoah, hold on a second. The default year on Jodatime is 2000, so fixed = date.minusYears(1); will set the year to 1999. I think what we need here is fixed=fixed.minusYears(1) (and the same for the plusYears).
        Hide
        hshreedharan Hari Shreedharan added a comment -

        Ah, I get your concern. Since we never do never set the year in the date object, it should be fixed.minusYears and fixed.plusYears...

        Roshan Naik - I think that does make sense. I think we need to fix this, correct?

        Show
        hshreedharan Hari Shreedharan added a comment - Ah, I get your concern. Since we never do never set the year in the date object, it should be fixed.minusYears and fixed.plusYears... Roshan Naik - I think that does make sense. I think we need to fix this, correct?
        Hide
        tmgstev Tristan Stevens added a comment -

        Attaching a patch that reverts the problem and also adds in some unit tests. I think we've got the relevant cases covered.

        Show
        tmgstev Tristan Stevens added a comment - Attaching a patch that reverts the problem and also adds in some unit tests. I think we've got the relevant cases covered.
        Hide
        hshreedharan Hari Shreedharan added a comment -

        Tristan Stevens Thanks for the patch. Can you base the patch on current trunk, rather than before the last commit, so I can just directly commit this?

        Show
        hshreedharan Hari Shreedharan added a comment - Tristan Stevens Thanks for the patch. Can you base the patch on current trunk, rather than before the last commit, so I can just directly commit this?
        Hide
        roshan_naik Roshan Naik added a comment - - edited

        Thanks Tristan Stevens for catching that.

        I am revising the patch with the same fix applied to SyslogAvroEventSerializer.java.

        Not sure why there is duplication of logic.
        Since short on time (feb 26 already) i wont try to attempt to merge the two code pieces and upload another fix.

        Can you please review this v3 patch ?

        Show
        roshan_naik Roshan Naik added a comment - - edited Thanks Tristan Stevens for catching that. I am revising the patch with the same fix applied to SyslogAvroEventSerializer.java. Not sure why there is duplication of logic. Since short on time (feb 26 already) i wont try to attempt to merge the two code pieces and upload another fix. Can you please review this v3 patch ?
        Hide
        roshan_naik Roshan Naik added a comment -

        Hari Shreedharan i am thinking we revert/override the previous commit with this patch ?

        Show
        roshan_naik Roshan Naik added a comment - Hari Shreedharan i am thinking we revert/override the previous commit with this patch ?
        Hide
        hshreedharan Hari Shreedharan added a comment -

        Sounds good, Roshan Naik - lets revert the last one and commit this one. This one LGTM, so +1.

        I'd still wait for Tristan Stevens to take a look, since he caught the issue earlier.

        Show
        hshreedharan Hari Shreedharan added a comment - Sounds good, Roshan Naik - lets revert the last one and commit this one. This one LGTM, so +1. I'd still wait for Tristan Stevens to take a look, since he caught the issue earlier.
        Hide
        roshan_naik Roshan Naik added a comment -

        Yes will wait for Tristan.

        Trying to assess the impact this bug has on flume users. Could use some inputs here .. My assessment is below :
        1) SyslogAvroEventSerializer.java: Here it could cause bad data to be written out to the file
        2) SyslogParser.java : This looks like will apply a bad date in the timestamp header. That could then either end up causing events to go down the wrong destination if that header is used to determine the destination... or even bad data .. if the header value is somehow during serialization
        3) In both cases is the problem limited to adjusting leap year dates (Feb 29) only ? would be nice to see an example showing the problem.
        Thanks

        Show
        roshan_naik Roshan Naik added a comment - Yes will wait for Tristan. Trying to assess the impact this bug has on flume users. Could use some inputs here .. My assessment is below : 1) SyslogAvroEventSerializer.java: Here it could cause bad data to be written out to the file 2) SyslogParser.java : This looks like will apply a bad date in the timestamp header. That could then either end up causing events to go down the wrong destination if that header is used to determine the destination... or even bad data .. if the header value is somehow during serialization 3) In both cases is the problem limited to adjusting leap year dates (Feb 29) only ? would be nice to see an example showing the problem. Thanks
        Hide
        roshan_naik Roshan Naik added a comment -

        Shall wait for Tristan Stevens till end of day to see if he gets a chance to review this before committing.

        Show
        roshan_naik Roshan Naik added a comment - Shall wait for Tristan Stevens till end of day to see if he gets a chance to review this before committing.
        Hide
        tmgstev Tristan Stevens added a comment -

        Hi Roshan Naik, LGTM - sorry for the delay.

        In answer to your questions:

        1) Yes, there could be some unexpected behaviour when a 29 Feb is observed in a non-leap year.
        2) The timestamp written will be in epoch format, so I think it will always be valid. The way the timestamp is created (create a year-2000 timestamp and then apply the new year) avoids any chances of an unhandled exception (I think). My customer is doing HDFS bucketting based on the extracted timestamp, but:
        3) I couldn't recreate an example where the behaviour with the old code was undesirable - hence I wrote the additional unit tests. Certainly subtraction through a 29th February seemed to work okay, the only un-certain behaviour is when a 29 Feb occurs in a non-leap year. In which case setting to 28th February seems reasonable IMHO.

        So not sure that there was a bug to start with!

        But I'm happy for it to be committed.

        Show
        tmgstev Tristan Stevens added a comment - Hi Roshan Naik , LGTM - sorry for the delay. In answer to your questions: 1) Yes, there could be some unexpected behaviour when a 29 Feb is observed in a non-leap year. 2) The timestamp written will be in epoch format, so I think it will always be valid. The way the timestamp is created (create a year-2000 timestamp and then apply the new year) avoids any chances of an unhandled exception (I think). My customer is doing HDFS bucketting based on the extracted timestamp, but: 3) I couldn't recreate an example where the behaviour with the old code was undesirable - hence I wrote the additional unit tests. Certainly subtraction through a 29th February seemed to work okay, the only un-certain behaviour is when a 29 Feb occurs in a non-leap year. In which case setting to 28th February seems reasonable IMHO. So not sure that there was a bug to start with! But I'm happy for it to be committed.
        Hide
        hshreedharan Hari Shreedharan added a comment -

        Roshan Naik - Mind committing this?

        Show
        hshreedharan Hari Shreedharan added a comment - Roshan Naik - Mind committing this?
        Hide
        tmgstev Tristan Stevens added a comment - - edited

        I've been running through this again - the logic is completely screwed up, for most of the year you can get undesirable behaviour. Shall I raise it as a separate JIRA or shall we fix it here and now? I've got some new tests and a patch. I didn't realise this hadn't been committed yet.

        Show
        tmgstev Tristan Stevens added a comment - - edited I've been running through this again - the logic is completely screwed up, for most of the year you can get undesirable behaviour. Shall I raise it as a separate JIRA or shall we fix it here and now? I've got some new tests and a patch. I didn't realise this hadn't been committed yet.
        Hide
        roshan_naik Roshan Naik added a comment -

        oh i thought i had committed it... perhaps we can wait for the revision Tristan has in mind.

        Tristan Stevens please go ahead and reuse this JIRA.

        Show
        roshan_naik Roshan Naik added a comment - oh i thought i had committed it... perhaps we can wait for the revision Tristan has in mind. Tristan Stevens please go ahead and reuse this JIRA.
        Hide
        hshreedharan Hari Shreedharan added a comment -

        Tristan Stevens - sounds good. Like Roshan Naik said, go ahead and reuse this jira.

        Show
        hshreedharan Hari Shreedharan added a comment - Tristan Stevens - sounds good. Like Roshan Naik said, go ahead and reuse this jira.
        Hide
        tmgstev Tristan Stevens added a comment -

        The current algorithm is as follows:

        1. Set the year of the parsed date to this year.
        2. If that sets the time after now and indeed more than one month after now then set the year one year previous to now (this is good).
        3. else, if that sets the time to before now and indeed more than one month before now then set the year to one year in advance to now.

        The flaw is that given syslog is historic, it's quite possible to get logs that are more than one month old - much more likely than having logs 11 months in the future.

        So the code should read:

        3. else, if that sets the time to before now but not more than 11 months before now (implying that the time should be < 1 month in the future) only then set the year to one year in advance to now.

        This gives us a 12 months window of tolerance, 1 month ahead and 11 months prior.

        Illustration of current behaviour:

        //Clock is about to roll into 2020 - we should expect anything February-December to be 2019 and anything January to be 2020
        Current date is: 30/12/2019 13:14:15UTC

        dateString = "Mar 03 01:02:03"; - comes back as 2020
        dateString = "Jun 03 01:02:03"; - comes back as 2020
        dateString = "Oct 03 01:02:03"; - comes back as 2020

        //Clock is June 2020 - we should expect anything August-December to be 2019 and anything January to July to be 2020
        Current date is: 21/06/2020 13:14:15UTC

        dateString = "Mar 03 01:02:03"; - comes back as 2021
        dateString = "Jan 2 01:02:03"; - comes back as 2021
        dateString = "Jan 21 01:02:03"; - comes back as 2021

        I've written a patch that adds in unit tests for these cases and others, plus that fixes the code. I'm currently looking at whether SyslogUtils has a similar problem.

        Show
        tmgstev Tristan Stevens added a comment - The current algorithm is as follows: 1. Set the year of the parsed date to this year. 2. If that sets the time after now and indeed more than one month after now then set the year one year previous to now (this is good). 3. else, if that sets the time to before now and indeed more than one month before now then set the year to one year in advance to now. The flaw is that given syslog is historic, it's quite possible to get logs that are more than one month old - much more likely than having logs 11 months in the future. So the code should read: 3. else, if that sets the time to before now but not more than 11 months before now (implying that the time should be < 1 month in the future) only then set the year to one year in advance to now. This gives us a 12 months window of tolerance, 1 month ahead and 11 months prior. Illustration of current behaviour: //Clock is about to roll into 2020 - we should expect anything February-December to be 2019 and anything January to be 2020 Current date is: 30/12/2019 13:14:15UTC dateString = "Mar 03 01:02:03"; - comes back as 2020 dateString = "Jun 03 01:02:03"; - comes back as 2020 dateString = "Oct 03 01:02:03"; - comes back as 2020 //Clock is June 2020 - we should expect anything August-December to be 2019 and anything January to July to be 2020 Current date is: 21/06/2020 13:14:15UTC dateString = "Mar 03 01:02:03"; - comes back as 2021 dateString = "Jan 2 01:02:03"; - comes back as 2021 dateString = "Jan 21 01:02:03"; - comes back as 2021 I've written a patch that adds in unit tests for these cases and others, plus that fixes the code. I'm currently looking at whether SyslogUtils has a similar problem.
        Hide
        tmgstev Tristan Stevens added a comment -

        Attached patch which fixes the issue in the new multiport syslog source but also a worse problem in the original syslog sources.

        In the original sources the current year was simply added on, which is very bad at the turn of the year and has no consideration for any lag of today minus Jan1st days.

        Show
        tmgstev Tristan Stevens added a comment - Attached patch which fixes the issue in the new multiport syslog source but also a worse problem in the original syslog sources. In the original sources the current year was simply added on, which is very bad at the turn of the year and has no consideration for any lag of today minus Jan1st days.
        Hide
        tmgstev Tristan Stevens added a comment -

        Hari Shreedharan Roshan Naik Did you have a chance to look at the new patch?

        Show
        tmgstev Tristan Stevens added a comment - Hari Shreedharan Roshan Naik Did you have a chance to look at the new patch?
        Hide
        roshan_naik Roshan Naik added a comment -

        Sorry Tristan Stevens i have not. Wont be able to get to it for another week at least. Hari Shreedharan are u able to take a look ?

        Show
        roshan_naik Roshan Naik added a comment - Sorry Tristan Stevens i have not. Wont be able to get to it for another week at least. Hari Shreedharan are u able to take a look ?
        Hide
        tmgstev Tristan Stevens added a comment -

        Hi Roshan Naik and Hari Shreedharan - I'm a bit worried that what is committed onto trunk at the moment is a regression and will cause problems for any users using this (by setting the year to year 2000 +/- 1 year) - any chance we can take a look at some fix OR revert the previous patch?

        Show
        tmgstev Tristan Stevens added a comment - Hi Roshan Naik and Hari Shreedharan - I'm a bit worried that what is committed onto trunk at the moment is a regression and will cause problems for any users using this (by setting the year to year 2000 +/- 1 year) - any chance we can take a look at some fix OR revert the previous patch?
        Hide
        roshan_naik Roshan Naik added a comment -

        +1 .. am running the tests. Shall commit them once they pass.

        Show
        roshan_naik Roshan Naik added a comment - +1 .. am running the tests. Shall commit them once they pass.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit e6df16d782ae8917b443be81d6a5ad755e02f5c3 in flume's branch refs/heads/trunk from Roshan Naik
        [ https://git-wip-us.apache.org/repos/asf?p=flume.git;h=e6df16d ]

        FLUME-2889: Fixes to DateTime computations

        (Tristan Stevens via Roshan Naik)

        Show
        jira-bot ASF subversion and git services added a comment - Commit e6df16d782ae8917b443be81d6a5ad755e02f5c3 in flume's branch refs/heads/trunk from Roshan Naik [ https://git-wip-us.apache.org/repos/asf?p=flume.git;h=e6df16d ] FLUME-2889 : Fixes to DateTime computations (Tristan Stevens via Roshan Naik)
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 49a370a62450b654aed52ef680b7c39e22a74948 in flume's branch refs/heads/flume-1.7 from Roshan Naik
        [ https://git-wip-us.apache.org/repos/asf?p=flume.git;h=49a370a ]

        FLUME-2889: Fixes to DateTime computations

        (Tristan Stevens via Roshan Naik)

        Show
        jira-bot ASF subversion and git services added a comment - Commit 49a370a62450b654aed52ef680b7c39e22a74948 in flume's branch refs/heads/flume-1.7 from Roshan Naik [ https://git-wip-us.apache.org/repos/asf?p=flume.git;h=49a370a ] FLUME-2889 : Fixes to DateTime computations (Tristan Stevens via Roshan Naik)
        Hide
        roshan_naik Roshan Naik added a comment -

        Thanks Tristan Stevens for fixing this tricky issue !

        Show
        roshan_naik Roshan Naik added a comment - Thanks Tristan Stevens for fixing this tricky issue !
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Flume-trunk-hbase-1 #156 (See https://builds.apache.org/job/Flume-trunk-hbase-1/156/)
        FLUME-2889: Fixes to DateTime computations (roshan: http://git-wip-us.apache.org/repos/asf/flume/repo?p=flume.git&a=commit&h=e6df16d782ae8917b443be81d6a5ad755e02f5c3)

        • flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
        • flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java
        • flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Flume-trunk-hbase-1 #156 (See https://builds.apache.org/job/Flume-trunk-hbase-1/156/ ) FLUME-2889 : Fixes to DateTime computations (roshan: http://git-wip-us.apache.org/repos/asf/flume/repo?p=flume.git&a=commit&h=e6df16d782ae8917b443be81d6a5ad755e02f5c3 ) flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java

          People

          • Assignee:
            tmgstev Tristan Stevens
            Reporter:
            roshan_naik Roshan Naik
          • Votes:
            1 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development