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
Added support for Key Rotation. #24452
Added support for Key Rotation. #24452
Changes from all commits
6e22d9b
29822a1
4e396ab
32f8e12
e844f93
f9b695a
67502a3
825c3cd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
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.
What do other Java libraries use to model durations? we use string in JS because it's idiomatic for us, but Java might have a different way to model these types... consider checking with the service bus feature crew on how they model ISO8601 durations
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.
Eh feel free to ignore me if this is all internal / serialization / generated code... I really don't know Java 😄
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 the Java SDK we do have a preferred datetime type:
OffsetDateTime
. For serialization/deserialization internal classes can keep usingString
forexpiryTime
andLong
forcreatedOn
andupdatedOn
, but I think it would be better to useOffsetDateTime
in public classes like you suggest.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.
Does Java have a better type for dates that we can use instead of a
Long
?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.
See my other comment on this.
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.
As we discussed there might be a better type for this -
Duration
maybe or whatever servicebus queue is using. Whatever is idiomatic for Java to represent ISO8601 Durations.I also know that this isn't the publicly exposed class but just calling it out here 👍
Same for timeAfterCreate and timeBeforeExpiry
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 am refraining of using
Duration
for the time being since it doesn't play nice with durations longer than days. The alternativePeriod
class is not meant for anything other than days/months/years. I'd rather keep using a String for now and hear what our architects have to say about this when we do the API review.