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

Produce records with consistent timestamps #1455

Merged
merged 1 commit into from
Dec 11, 2019

Conversation

stevenjm
Copy link
Contributor

@stevenjm stevenjm commented Aug 8, 2019

It is possible for the same record to have a different timestamp depending on where it appears in a produceSet, as the test case in this commit illustrates.

The problem is that the produceSet's FirstTimestamp and the record's TimestampDelta are each calculated with nanosecond precision, and then truncated to millisecond precision during encoding. This leads to accumulated rounding error when the original timestamp is later reconstructed.

Instead, truncate all timestamps to millisecond precision before calculating the FirstTimestamp and TimestampDelta, so that if the same record is produced multiple times, it will always have the same timestamp.

It might be better to call encode on each record instead of reproducing the logic of how to encode a timestamp in the test, but I can't see an easy way to get the result of encoding a record. I'm open to feedback on how to improve the test.

It is possible for the same record to have a different timestamp
depending on where it appears in a produceSet, as the test case in
this commit illustrates.

The problem is that the produceSet's FirstTimestamp and the
record's TimestampDelta are each calculated with nanosecond
precision, and then truncated to millisecond precision during
encoding. This leads to accumulated rounding error when the
original timestamp is later reconstructed.

Instead, truncate all timestamps to millisecond precision before
calculating the FirstTimestamp and TimestampDelta, so that if the
same record is produced multiple times, it will always have the
same timestamp.
@ghost ghost added the cla-needed label Aug 8, 2019
@bai bai requested a review from varun06 August 16, 2019 04:27
Copy link
Contributor

@varun06 varun06 left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@bai
Copy link
Contributor

bai commented Aug 17, 2019

Thanks @stevenjm; could you please sign CLA 🙏

@stevenjm
Copy link
Contributor Author

Thanks @stevenjm; could you please sign CLA 🙏

I've already requested my employer sign the corporate CLA; that should hopefully be done soon.

@bai
Copy link
Contributor

bai commented Oct 28, 2019

👋 do you happen to have any news about this one? 🙏

@stevenjm
Copy link
Contributor Author

Apologies for the delay! Signing the CLA has been approved in principle and should hopefully be done this week.

@renatomefi
Copy link

Hello, we've signed the CLA, how can we re-trigger the check?

@renatomefi
Copy link

@stevenjm maybe you can amend the commit and force push to get a new trigger from GH

@varun06 varun06 closed this Dec 11, 2019
@varun06 varun06 reopened this Dec 11, 2019
@ghost ghost removed the cla-needed label Dec 11, 2019
@varun06 varun06 merged commit d514254 into IBM:master Dec 11, 2019
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.

4 participants