-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
rls: overhaul RouteLookupConfig validation #8645
Conversation
d0931c7
to
3b1f2a3
Compare
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 I caught some potential NullPointerException
s.
this.maxAgeInNanos = maxAgeInNanos; | ||
this.staleAgeInNanos = staleAgeInNanos; |
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.
Potential error: Unboxing LongmaxAgeInNanos
and staleAgeInNanos
into long this.maxAgeInNanos
and this.staleAgeInNanos
may produce 'NullPointerException'.
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 is an oversight. Fixed to use primitive long
type.
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.
Good catches! Thanks for meticulously reviewing the PR.
this.maxAgeInNanos = maxAgeInNanos; | ||
this.staleAgeInNanos = staleAgeInNanos; |
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 is an oversight. Fixed to use primitive long
type.
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.
Looks good. Minor comments + you missed one on the deleted code: #8645 (comment)
This PR fixes a few cosmetic violations of the ErrorProne patterns introduced in PR grpc#8645: ParameterName, and TimeUnitMismatch.
This PR fixes a few cosmetic violations of the ErrorProne patterns introduced in PR #8645: ParameterName, and TimeUnitMismatch.
The
RlsProtoData.RouteLookupConfig
class is out-of-date.Duration
type.Now overhaul all the fields in
RlsProtoData.RouteLookupConfig
class based on the spec http://go/grpc-rls-lb-policy-design#heading=h.y3h669gfpown.Also move the validation logic in json parsing rather than in the constructor of
RouteLookupConfig
.