Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

negative timestamps on messages fix #907

Merged
merged 1 commit into from
Jun 30, 2017
Merged

negative timestamps on messages fix #907

merged 1 commit into from
Jun 30, 2017

Conversation

SamiHiltunen
Copy link
Contributor

  • Adds checks for negative timestamps on messages. Negative
    timestamps are invalid. On decoding, return a zero time for easy
    existence checking. On encoding, set timestamp to be -1 if the
    given time describes a time before unix epoch.

Copy link
Contributor

@eapache eapache left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. I'm wondering if we should return errors instead of just masking out the bad data though? It's what we do for other invalid fields.

@SamiHiltunen
Copy link
Contributor Author

I assume you mean to return an error when trying to encode a bad packet? Kafka is returning messages with -1 timestamps so the decoding part should probably just return a zero time if it encounters anything below 0. For encoding, returning errors would make sense.

I'll make the following changes for the encoding part:

  1. Except for zero time, anything before unix epoch will return a PacketEncodingError{"invalid timestamp"}
  2. If timestamp is a zero time, timestamp will be set to -1
  3. Time value on or after unix epoch will use the current formula

Does this sound good?

message.go Outdated
pe.putInt64(m.Timestamp.UnixNano() / int64(time.Millisecond))
timestamp := int64(-1)

if m.Timestamp.After(time.Unix(0, -1)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking at the actual implementation (https://golang.org/src/time/time.go?s=29125:29162#L991), did you mean time.Unix(0, 0)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or perhaps it would be simpler to just do !Before a zero time, which is cleaner to construct

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I inverted the condition to use !m.Timestamp.Before as you suggested. Let me know if there is anything else.

* Negative timestamps on messages are invalid thus this commit adds
  checks for them. On decoding, this returns a zero time for
  negative timestamps for easy validity checking. On encoding, zero
  times will be encoded as -1, anything else before Unix epoch
  returns an error.
@eapache eapache merged commit d81f250 into IBM:master Jun 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants