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

Avoid MIN_VALUE and MAX_VALUE from Timestamps #1544

Merged
merged 3 commits into from
Nov 14, 2023

Conversation

armiol
Copy link
Contributor

@armiol armiol commented Nov 13, 2023

This changeset addresses test code, which used easily-reached constants Timestamps.MIN_VALUE and Timestamps.MAX_VALUE.

The problem with using these constants is that on a Storage level, we convert those to nanoseconds — as it is a part of our mechanism on providing signal ordering within a single JVM (see emulated nanos in Time). But the mentioned constants defined by Timestamps cannot be converted to nanos, as the proposed Java type is long, and it is not long enough to store such values.

Ultimately, in Storage implementations, using these constants led to long overflow.

Besides, MIN_VALUE corresponds to 1 CE, and MAX_VALUE corresponds to year 9999. Both of which aren't widely used in event-sourced applications at the moment. We'll see how it goes, but for now it's definitely an overkill.

This PR migrates the test code to using more meaningful Timestamp values.

…their conversion to nanoseconds (which we need for proper ordering of e.g. Events) leads to `long` type overflow.

Review the related tests for sanity.
@armiol armiol self-assigned this Nov 13, 2023
@armiol
Copy link
Contributor Author

armiol commented Nov 13, 2023

@alexander-yevsyukov PTAL.

Copy link
Contributor

@alexander-yevsyukov alexander-yevsyukov left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

codecov bot commented Nov 13, 2023

Codecov Report

Merging #1544 (8967363) into master (e00ad8f) will increase coverage by 0.02%.
The diff coverage is n/a.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1544      +/-   ##
============================================
+ Coverage     89.81%   89.84%   +0.02%     
  Complexity     5025     5025              
============================================
  Files           645      645              
  Lines         15751    15751              
  Branches        920      920              
============================================
+ Hits          14147    14151       +4     
+ Misses         1276     1274       -2     
+ Partials        328      326       -2     

@armiol armiol merged commit e489ca2 into master Nov 14, 2023
7 checks passed
@armiol armiol deleted the timestamps-in-aggregate-tests branch November 14, 2023 10:42
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