goiri commented on a change in pull request #1904:
URL: https://github.com/apache/hadoop/pull/1904#discussion_r396022750
##########
File path:
hadoop-common-project/hadoop-auth/src/test/java/org/apache/hadoop/security/authentication/util/TestRolloverSignerSecretProvider.java
##########
@@ -20,7 +20,7 @@
@Test
public void testGetAndRollSecrets() throws Exception {
- long rolloverFrequency = 15 * 1000; // rollover every 15 sec
+ long rolloverFrequency = 2 * 1000; // rollover every 2 sec
Review comment:
We only use it in secretProvider#init() right?
In that case, let's just set it there and specify as:
secretProvider.init(null, null, TimeUnit.SECONDS.toMillis(2));
##########
File path:
hadoop-common-project/hadoop-auth/src/test/java/org/apache/hadoop/security/authentication/util/TestRolloverSignerSecretProvider.java
##########
@@ -36,23 +36,26 @@ public void testGetAndRollSecrets() throws Exception {
Assert.assertEquals(2, allSecrets.length);
Assert.assertArrayEquals(secret1, allSecrets[0]);
Assert.assertNull(allSecrets[1]);
- Thread.sleep(rolloverFrequency + 2000);
+ synchronized (secretProvider.monitor){
Review comment:
What about wrapping this into a function VisibleForTesting in
TRolloverSignerSecretProvider?
##########
File path:
hadoop-common-project/hadoop-auth/src/test/java/org/apache/hadoop/security/authentication/util/TestRolloverSignerSecretProvider.java
##########
@@ -62,16 +65,19 @@ public void testGetAndRollSecrets() throws Exception {
private byte[][] newSecretSequence;
private int newSecretSequenceIndex;
+ final Object monitor = new Object();
Review comment:
Make it private and add a short /** javadoc. */
##########
File path:
hadoop-common-project/hadoop-auth/src/test/java/org/apache/hadoop/security/authentication/util/TestRolloverSignerSecretProvider.java
##########
@@ -52,7 +52,6 @@ public void testGetAndRollSecrets() throws Exception {
Assert.assertEquals(2, allSecrets.length);
Assert.assertArrayEquals(secret3, allSecrets[0]);
Assert.assertArrayEquals(secret2, allSecrets[1]);
- Thread.sleep(rolloverFrequency + 2000);
Review comment:
Correct, that's the kind of sync I was looking for.
##########
File path:
hadoop-common-project/hadoop-auth/src/test/java/org/apache/hadoop/security/authentication/util/TestRolloverSignerSecretProvider.java
##########
@@ -62,16 +65,19 @@ public void testGetAndRollSecrets() throws Exception {
private byte[][] newSecretSequence;
private int newSecretSequenceIndex;
+ final Object monitor = new Object();
Review comment:
It could use a more specific name too.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
