bbotella commented on code in PR #3706:
URL: https://github.com/apache/cassandra/pull/3706#discussion_r1901151341


##########
src/java/org/apache/cassandra/auth/CassandraRoleManager.java:
##########
@@ -732,4 +747,76 @@ public Supplier<Map<RoleResource, Set<Role>>> bulkLoader()
             return entries;
         };
     }
+
+    protected void disconnectInvalidRoles()
+    {
+        // This should always run with jitter, otherwise there's a risk that 
all nodes disconnect clients at the same time
+        StorageService.instance.disconnectInvalidRoles();
+    }
+
+    protected void invalidRoleDisconnectTask(LongSupplier delayMillis, 
ScheduledExecutorService executor)
+    {
+        try
+        {
+            disconnectInvalidRoles();
+        }
+        catch (Exception e)
+        {
+            logger.warn("Failed to disconnect invalid roles", e);
+        }
+
+        long nextDelayMillis = delayMillis.getAsLong();
+        logger.info("Scheduling next invalid role disconnection in {} millis", 
nextDelayMillis);
+        this.invalidRoleDisconnectTask = executor.schedule(() -> 
invalidRoleDisconnectTask(delayMillis, executor), nextDelayMillis, 
TimeUnit.MILLISECONDS);
+    }
+
+    protected void scheduleDisconnectInvalidRoleTask()
+    {
+        // Cancel any pending execution if it exists, since we may have 
changed period / jitter parameters
+        if (this.invalidRoleDisconnectTask != null)
+        {
+            logger.debug("Canceling previous invalidRoleDisconnectTask");
+            this.invalidRoleDisconnectTask.cancel(true);
+        }
+
+        long period = getInvalidClientDisconnectPeriodMillis();
+        long jitter = getInvalidClientDisconnectJitterMillis();

Review Comment:
   Nit: Maybe get Jitter after the `<=` check for period?



##########
test/unit/org/apache/cassandra/auth/CassandraRoleManagerTest.java:
##########
@@ -160,4 +168,38 @@ private void assertRoleSet(Set<Role> actual, 
RoleResource...expected)
         for (RoleResource expectedRole : expected)
             assertTrue(actual.stream().anyMatch(role -> 
role.resource.equals(expectedRole)));
     }
+
+    @Test
+    public void example() throws InterruptedException

Review Comment:
   Maybe find a more descriptive name? Something like 
`numDisconnectAttempsTest`?



-- 
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]

Reply via email to