nastra commented on code in PR #6562:
URL: https://github.com/apache/iceberg/pull/6562#discussion_r1073295311
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java:
##########
@@ -461,8 +463,9 @@ private OAuthTokenResponse refreshCurrentToken(RESTClient
client) {
}
}
- static boolean isExpired(Long expiresAtMillis, long now) {
- return expiresAtMillis != null && expiresAtMillis < now;
+ static boolean isExpired(Instant expiresAt) {
Review Comment:
@rdblue you mentioned in
https://github.com/apache/iceberg/pull/6489#discussion_r1072451113 that this
type of method can be misleading, because the token could still be expired even
though `expiresAt` is `null`.
To me it makes sense to have this method, given that we're using it at 2
different places.
Technically we're just checking whether a given `Instant` is expired or not
so IMO it's clear what the purpose of this method is.
I can't come up with a better name, but I'm happy to rename it to something
more obvious in case you have suggestions.
Would `isNonNullAndExpired()` be more descriptive about the purpose of the
method?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
