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

minor patch ... ready for review

+1. LGTM.

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

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)

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)

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

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).

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?

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

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?

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 ?

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

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.

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

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

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.

Roshan Naik - Mind committing this?

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.

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.

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

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.

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.

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

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 ?

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?

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

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)

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)

Thanks Tristan Stevens for fixing this tricky issue !

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