josh-mckenzie commented on a change in pull request #1213:
URL: https://github.com/apache/cassandra/pull/1213#discussion_r724246464
##########
File path: src/java/org/apache/cassandra/schema/PartitionDenylist.java
##########
@@ -161,36 +159,35 @@ private boolean readAllHasSufficientNodes()
/** Helper method as we need to both build cache on initial init but also
on reload of cache contents and params */
private LoadingCache<TableId, DenylistEntry> buildEmptyCache()
{
- return CacheBuilder.newBuilder()
-
.refreshAfterWrite(DatabaseDescriptor.getDenylistRefreshSeconds(),
TimeUnit.SECONDS)
- .build(new CacheLoader<TableId, DenylistEntry>()
+ return Caffeine.newBuilder()
+
.refreshAfterWrite(DatabaseDescriptor.getDenylistRefreshSeconds(),
TimeUnit.SECONDS)
+ .executor(executor)
+ .build(new CacheLoader<TableId, DenylistEntry>()
+ {
+ @Override
+ public DenylistEntry load(final TableId tid) throws
Exception
{
- @Override
- public DenylistEntry load(final TableId tid)
throws Exception
- {
- return getDenylistForTable(tid);
- }
-
- @Override
- public ListenableFuture<DenylistEntry>
reload(final TableId tid, final DenylistEntry oldValue)
- {
- ListenableFutureTask<DenylistEntry> task =
ListenableFutureTask.create(new Callable<DenylistEntry>()
- {
- @Override
- public DenylistEntry call()
- {
- final DenylistEntry newEntry =
getDenylistForTable(tid);
- if (newEntry != null)
- return newEntry;
- if (oldValue != null)
- return oldValue;
- return new DenylistEntry();
- }
- });
- executor.execute(task);
- return task;
- }
- });
+ return getDenylistForTableFromCQL(tid);
+ }
+
+ @Override
+ public CompletableFuture<DenylistEntry>
asyncReload(final TableId tid, final DenylistEntry oldValue, Executor executor)
+ {
+ return CompletableFuture.supplyAsync(() -> {
+ /* We accept DenylistEntry data in the
following precedence:
+ 1) new data pulled from CQL if we got
it correctly,
+ 2) the old value from the cache if
there was some error querying,
+ 3) an empty record otherwise.
+ */
+ final DenylistEntry newEntry =
getDenylistForTableFromCQL(tid);
+ if (newEntry != null)
+ return newEntry;
+ if (oldValue != null)
+ return oldValue;
+ return new DenylistEntry();
+ }, executor);
+ }
Review comment:
The thing that wasn't clear to me was the execution context (i.e. thread
pool / executor etc) that sync reload calls occurred on. So long as that's on
the provided executor for the builder, strong :+1: to tidying this up.
##########
File path: conf/cassandra.yaml
##########
@@ -992,6 +992,34 @@ slow_query_log_timeout_in_ms: 500
# bound (for example a few nodes with big files).
# streaming_connections_per_host: 1
+# Allows denying configurable access (rw/rr) to operations on configured ks,
table, and partitions, intended for use by
+# operators to manage cluster health vs application access. See
CASSANDRA-12106 and CEP-13 for more details.
+# enable_partition_denylist = false;
+
+# enable_denylist_writes = true;
+# enable_denylist_reads = true;
+# enable_denylist_range_reads = true;
+
+# The interval at which keys in the cache for denylisting will "expire" and
async refresh from the backing DB.
+# denylist_refresh_seconds = 86400;
+
+# In the event of errors on attempting to load the denylist cache, retry on
this interval.
+# denylist_initial_load_retry_seconds = 5;
+
+# We cap the number of denylisted keys allowed per table to keep things from
growing unbounded. Nodes will warn above
+# this limit while allowing new denylisted keys to be inserted. Denied keys
are loaded in natural query / clustering
+# ordering by partition key in case of overflow.
+# max_denylist_keys_per_table = 1000;
Review comment:
If it isn't obvious by now, playing whack-a-mole trying to fix the old
naming from someone else's old code is... fraught. Thanks for catching this.
I'll do another pass across all the yaml + Config.java + DBD stuff after I go
through these revisions.
##########
File path: conf/cassandra.yaml
##########
@@ -992,6 +992,34 @@ slow_query_log_timeout_in_ms: 500
# bound (for example a few nodes with big files).
# streaming_connections_per_host: 1
+# Allows denying configurable access (rw/rr) to operations on configured ks,
table, and partitions, intended for use by
+# operators to manage cluster health vs application access. See
CASSANDRA-12106 and CEP-13 for more details.
+# enable_partition_denylist = false;
+
+# enable_denylist_writes = true;
+# enable_denylist_reads = true;
+# enable_denylist_range_reads = true;
+
+# The interval at which keys in the cache for denylisting will "expire" and
async refresh from the backing DB.
+# denylist_refresh_seconds = 86400;
+
+# In the event of errors on attempting to load the denylist cache, retry on
this interval.
+# denylist_initial_load_retry_seconds = 5;
+
+# We cap the number of denylisted keys allowed per table to keep things from
growing unbounded. Nodes will warn above
+# this limit while allowing new denylisted keys to be inserted. Denied keys
are loaded in natural query / clustering
+# ordering by partition key in case of overflow.
+# max_denylist_keys_per_table = 1000;
Review comment:
If it isn't obvious by now, playing whack-a-mole trying to fix the old
naming from someone else's old code is... fraught. Thanks for catching this.
I'll do another pass across all the yaml + Config.java + DBD stuff after I go
through these revisions.
edit: incidentally this looks like a case of idea not catching it on the
refactor->rename.
##########
File path: src/java/org/apache/cassandra/config/DatabaseDescriptor.java
##########
@@ -3447,6 +3447,105 @@ public static void
setConsecutiveMessageErrorsThreshold(int value)
conf.consecutive_message_errors_threshold = value;
}
+ public static boolean getPartitionDenylistEnabled()
Review comment:
Updated this, .yaml params, config names, and method names to all be
coherent.
##########
File path: src/java/org/apache/cassandra/schema/SystemDistributedKeyspace.java
##########
@@ -68,6 +62,8 @@ private SystemDistributedKeyspace()
{
}
+ public static final String NAME = "system_distributed";
Review comment:
Good catch. This was used in a couple of places in PartitionDenylist
(the original implementation is using a different new Keyspaces entirely and
part of this patch was moving it from there to System Distributed); this is a
needless redundancy. Removed.
##########
File path: test/unit/org/apache/cassandra/config/DatabaseDescriptorTest.java
##########
@@ -590,4 +591,32 @@ public void
testApplyTokensConfigInitialTokensOneNumTokensNotSet()
Assert.assertEquals(Integer.valueOf(1), config.num_tokens);
Assert.assertEquals(1,
DatabaseDescriptor.tokensFromString(config.initial_token).size());
}
+
+ @Test
+ public void testDenylistInvalidValuesRejected()
+ {
+ DatabaseDescriptor.loadConfig();
+
+
expectIllegalArgumentException(DatabaseDescriptor::setDenylistRefreshSeconds,
0);
+
expectIllegalArgumentException(DatabaseDescriptor::setDenylistRefreshSeconds,
-1);
+
+
expectIllegalArgumentException(DatabaseDescriptor::setDenylistKeysPerTableMax,
0);
+
expectIllegalArgumentException(DatabaseDescriptor::setDenylistKeysPerTableMax,
-1);
+
+
expectIllegalArgumentException(DatabaseDescriptor::setDenylistKeysTotalMax, 0);
+
expectIllegalArgumentException(DatabaseDescriptor::setDenylistKeysTotalMax, -1);
+ }
+
+ private void expectIllegalArgumentException(Consumer<Integer> c, int val)
Review comment:
Ah. Wasn't familiar w/that - tidies that up nicely and augments it with
the param name checking too. :+1:
--
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]
