Add support for epoch timestamps and configurable output format #3860
Conversation
Signed-off-by: Krishna Kondaka <[email protected]>
kkondaka
requested review from
chenqi0805,
engechas,
graytaylor0,
dinujoh,
asifsmohammed,
dlvenable and
oeyh
as code owners
| @@ -38,6 +38,7 @@ default void parse( | |||
| Consumer<Record<Event>> eventConsumer) throws IOException { | |||
| Objects.requireNonNull(inputFile); | |||
| Objects.requireNonNull(eventConsumer); | |||
| System.out.println("======InputFile==="+inputFile); | |||
| long epochMillis = 0L; | ||
| long epochNanos = 0L; | ||
| ZonedDateTime zonedDateTime; | ||
| if (inputFormat.equals("epoch_second")) { |
There was a problem hiding this comment.
Generally, you should avoid conditionals in tests. It becomes harder to know if your test is correct or not.
You can split this into multiple tests. Or, you can refactor to include the input and expected output data as arguments.
| } | ||
| if (numberValue != null) { | ||
| int timestampLength = sourceTimestamp.length(); | ||
| if (timestampLength > LENGTH_OF_EPOCH_IN_MILLIS) { |
There was a problem hiding this comment.
I think we should invert these conditionals. Let's look at the list of epoch_ values the user configures and then find the best fit from there.
This currently starts by inferring it and then seeing if the user has that. But, what if the value is very far in time from what we would typically expect?
There was a problem hiding this comment.
I think this is similar to providing two patterns like ["dd-mm-YYYY", "YYYY-mm-dd"] and the input not matching either one. Similarly, if the input has 13 digits and the pattern is ["epoch_second"] we should not match it. Basically, the pattern "epoch_second" should be treated as matching for 10-digit number, "epoch_milli" should be treated as matching for 13-digit number, and "epoch_nano" should be treated as matching 19-digit number. Because any other matching could result in silent errors.
There was a problem hiding this comment.
We discussed offline. We will be more lenient on going back in time, but not forward in time. And we will only allow the selection of only one of the epoch_ configurations. Users won't select multiple of these for now.
There was a problem hiding this comment.
The current code already allows epoch times from the past but not from the future. Added check to make sure only one of the three "epoch_" patterns are allowed.
Signed-off-by: Krishna Kondaka <[email protected]>
Signed-off-by: Krishna Kondaka <[email protected]>
| } | ||
| return true; | ||
| } | ||
|
|
||
| @AssertTrue(message = "match can have a minimum and maximum of 1 entry and at least one pattern.") |
There was a problem hiding this comment.
Is this message meant to reference output format being invalid?
Signed-off-by: Krishna Kondaka <[email protected]>
* Add support for epoch timestamps and configurable output format Signed-off-by: Krishna Kondaka <[email protected]> * Add support for epoch timestamps and configurable output format Signed-off-by: Krishna Kondaka <[email protected]> * Addressed review comments Signed-off-by: Krishna Kondaka <[email protected]> * Addressed review comments Signed-off-by: Krishna Kondaka <[email protected]> --------- Signed-off-by: Krishna Kondaka <[email protected]> Co-authored-by: Krishna Kondaka <[email protected]> (cherry picked from commit f19de03)
Description
Supports input timestamp to be of
epoch_second,epoch_milliorepoch_nano.Also added support to configure output timestamp format. It can be also epoch timestamp or any of the formats allowed by DateTimeFormatter.
Resolves #2929
Issues Resolved
Resolves #2929
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.