Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: introduce
java.time
#2415feat: introduce
java.time
#2415Changes from 37 commits
71aa9a8
62dd45f
959a469
ca275dc
a7746ad
321c952
d25d6ef
796aa4b
a7e6167
0072191
99600a6
3765977
38b13e9
f6efe6b
41bc6ea
e06a104
fc920b0
9b6927a
987c343
173d961
de505d2
dc19a86
ff1b0fa
5b6d8af
efe806b
812c5a5
7deadf3
e2b1621
dde9326
6122ff1
70d6712
413703d
21c4614
253d351
ad9fa48
07c939a
d13c96b
540b19d
ce6a279
51657d8
06501e7
ed0213d
5994e4f
7ce813f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to remove the threeten setter, only store it as java.time.Instant, and then reverse the overload of getCommitTime(stamp) above.
The builder is fully internal, we just need to maintain backwards compatibility for the getter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the catch. This was indeed stated in the tracking sheet. I'll update this as an internal class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as for commit time. Also if we need to keep both setters this should be 'setEstimatedLowWatermarkTime'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setEstimatedLowWatermarkInstant should be setEstimatedLowWatermarkTime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be removed since it's an internal api change as per #2415 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as in ChangeStreamMutation. Wouldn't it be better to store this as java.time.Instant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In ChangeStreamMutation we call this
getEstimatedLowWatermarkTime
. We should be consistent w that naming hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed as this is an internal api. I only kept the java.time version