You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/11/30 23:25:42 UTC
[GitHub] [accumulo] cshannon opened a new pull request, #3099: Fix flaky AuthenticationTokenSecretManagerTest
cshannon opened a new pull request, #3099:
URL: https://github.com/apache/accumulo/pull/3099
Part of the ID of the generated token is the issue date which is the current time in millis so this adds a 1 millisecond sleep in between token generation to guarantee the tokens will be unique.
This fixes #3075
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon merged pull request #3099: Fix flaky AuthenticationTokenSecretManagerTest
Posted by GitBox <gi...@apache.org>.
cshannon merged PR #3099:
URL: https://github.com/apache/accumulo/pull/3099
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #3099: Fix flaky AuthenticationTokenSecretManagerTest
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #3099:
URL: https://github.com/apache/accumulo/pull/3099#issuecomment-1333304308
> @ctubbsii - not sure what's up but the recent checks are failing due to "shellcheck" having an issue. It looks like the last 3 commits have had the same problem starting with the Apache parent pom version 28 update so not sure if it's related to the root pom change or a coincidence.
@cshannon The changes where this was first observed did not change anything even remotely related to the scripts that would have triggered shellcheck. It looks like the version of shellcheck installed on the builder is newer than the version that we've previously seen, and the newer version is detecting a new issue, https://www.shellcheck.net/wiki/SC2294
This is easily fixed. Please disregard for now, since you're not changing any scripts.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #3099: Fix flaky AuthenticationTokenSecretManagerTest
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #3099:
URL: https://github.com/apache/accumulo/pull/3099#issuecomment-1332890525
@ctubbsii - not sure what's up but the recent checks are failing due to "shellcheck" having an issue. It looks like the last 3 commits have had the same problem starting with the Apache parent pom version 28 update so not sure if it's related to the root pom change or a coincidence.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3099: Fix flaky AuthenticationTokenSecretManagerTest
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #3099:
URL: https://github.com/apache/accumulo/pull/3099#discussion_r1036753726
##########
server/base/src/test/java/org/apache/accumulo/server/security/delegation/AuthenticationTokenSecretManagerTest.java:
##########
@@ -208,6 +208,8 @@ public void testVerifyPassword() throws Exception {
assertArrayEquals(password, secretManager.retrievePassword(id));
// Make a second token for the same user
+ // Sleep for 1 millisecond to guarantee token is unique
+ Thread.sleep(100);
Review Comment:
The comment doesn't match. This is sleeping for 100 milliseconds, right? Other than that mismatch, this seems fine.
```suggestion
// Briefly sleep to guarantee token is unique, since the token is based on the time
Thread.sleep(100);
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on a diff in pull request #3099: Fix flaky AuthenticationTokenSecretManagerTest
Posted by GitBox <gi...@apache.org>.
cshannon commented on code in PR #3099:
URL: https://github.com/apache/accumulo/pull/3099#discussion_r1036966840
##########
server/base/src/test/java/org/apache/accumulo/server/security/delegation/AuthenticationTokenSecretManagerTest.java:
##########
@@ -208,6 +208,8 @@ public void testVerifyPassword() throws Exception {
assertArrayEquals(password, secretManager.retrievePassword(id));
// Make a second token for the same user
+ // Sleep for 1 millisecond to guarantee token is unique
+ Thread.sleep(100);
Review Comment:
Yeah I updated the time from 1 to 100 in my second commit but didn't update the comment so good catch.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org