beobal commented on code in PR #3240:
URL: https://github.com/apache/cassandra/pull/3240#discussion_r1559123074
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -5153,4 +5153,15 @@ public static int
getSaiSSTableIndexesPerQueryFailThreshold()
{
return conf.sai_sstable_indexes_per_query_fail_threshold;
}
+
+ public static void setTriggersPolicy(Config.TriggersPolicy policy)
Review Comment:
can we annotate this `@VisibleForTesting` please?
##########
src/java/org/apache/cassandra/triggers/TriggerExecutor.java:
##########
@@ -220,6 +230,16 @@ private List<Mutation> executeInternal(PartitionUpdate
update)
Triggers triggers = update.metadata().triggers;
if (triggers.isEmpty())
return null;
+ Config.TriggersPolicy policy = DatabaseDescriptor.getTriggersPolicy();
Review Comment:
> at least DatabaseDescriptor.getTriggersPolicy() might be "cached" so we do
not need to call it all over again.
As the policy is effectively immutable, this is almost equivalent to a
constant lookup. Caching it isn't going to give us much in performance terms,
and it won't do anything for code clarity. If it were memoized on
`TriggersExecutor`, tests would need a way to set/override it, so for the sake
of clarity I'd suggest leaving it as is.
> I just feel like we are burying this logic in some executeInternal method
instead of failing as the very first thing we have a chance to do that.
This is the first point at which we inspect the update to check whether the
update actually has any triggers attached, so it seems a reasonable place to do
this validation. It's intrinsic to the execution of any triggers, so I'd say
the method that encapsulates that (i.e. `executeInternal`) is the ideal place
for it.
> I would also move it to execute method, outside of the for loop where
executeInternal is executed, because we would unnecessarily go through this
logic all over again calling warn on logger (even it is non-spamming one).
Triggers are associated with a specific `PartitionUpdate` so in the version
of `execute` which takes a collection, this has to be checked for each
mutation.
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -1300,4 +1300,16 @@ public static void log(Config config)
public volatile DurationSpec.LongMillisecondsBound
progress_barrier_backoff = new DurationSpec.LongMillisecondsBound("1000ms");
public volatile DurationSpec.LongSecondsBound discovery_timeout = new
DurationSpec.LongSecondsBound("30s");
public boolean unsafe_tcm_mode = false;
+
+ public enum TriggersPolicy
+ {
+ // Execute triggers
+ enabled,
+ // Don't execute triggers when executing queries
+ disabled,
+ // Throw an exception when attempting to execute a trigger
+ forbidden
+ }
+
+ public volatile TriggersPolicy triggers_policy = TriggersPolicy.enabled;
Review Comment:
Probably doesn't need to be volatile
--
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]
