HADOOP-18675: Fix CachedSASToken noisy log errors when SAS token has YYYY-MM-DD expiration #5508
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
requested changes
Mar 23, 2023
Contributor
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
makes sense. added some comments. I'm also going to reinstate the bit from the PR template which asks the submitter which storage endpoint they used.
see testing azure
| // set first time and ensure reference equality | ||
| cachedSasToken.update(token); | ||
| String cachedToken = cachedSasToken.get(); | ||
| Assert.assertTrue(token == cachedToken); |
Contributor
There was a problem hiding this comment.
assertEquals(expected, actual) or AssertJ.asserThat(). thanks
Author
There was a problem hiding this comment.
was following the existing tests which seem to want to assert reference equality a lot, I will change it for the new test.
Contributor
There was a problem hiding this comment.
yeah, old code...just because it was done that way doesn't mean we should carry on
| } | ||
| } catch (DateTimeParseException dateOnlyException) { | ||
| // log original exception | ||
| LOG.error("Error parsing se query parameter ({}) from SAS as ISO_DATE_TIME or ISO_DATE.", seValue, dateTimeException); |
Contributor
There was a problem hiding this comment.
now, if there's a problem here it is still going to log a lot, isn't it?
might be best to use a LogExactlyOnce log here for the error line, and log at debug the rest of the time
| } | ||
|
|
||
| @Test | ||
| public void testValidExpirationParsing() { |
Contributor
There was a problem hiding this comment.
is there a test for an invalid date time parsing now?
Author
There was a problem hiding this comment.
there's an existing one testing invalid se=abc in testUpdateAndGetWithInvalidToken. I'll add another method specifically for invalid expiration.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
| } else { | ||
| throw dateTimeException; | ||
| } | ||
| } catch (DateTimeParseException dateOnlyException) { |
Contributor
There was a problem hiding this comment.
Can we have something like:
private static final DateTimeFormatter[] formatters;
private OffsetDateTime getParsedDateTime(String dateTime) {
for(DateTimeFormatter formatter : formatters) {
try {
TemporalAccessor temporalAccessor = formatter.parseBest(dateTime);
if(temporalAccessor instanceof OffsetDateTime) {
return (OffsetDateTime) temporalAccessor;
}
if(temporalAccessor instanceof LocalDateTime) {
return ((LocalDateTime) temporalAccessor).atOffset(ZoneOffset.UTC);
}
} catch (DateTimeParseException e) {
}
}
return null;
}
And from this method we will just call seDate = getParsedDateTime(seValue);
This way I feel we can prevent nested code.
Contributor
|
We're closing this stale PR because it has been open for 100 days with no activity. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Description of PR
When using SAS tokens with expiration dates in the format YYYY-MM-DD, a frequent error appears in the logs. The cause is that the code expects ISO_DATE_TIME, but ISO_DATE type formats are acceptable as well.
How was this patch tested?
Added tests that the expiration is properly parsed using the previous failing format.
Ran
mvn test -Dtest="TestCachedSASToken"For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?