[GitHub] [cassandra] ekaterinadimitrova2 commented on a change in pull request #1444: CASSANDRA-17187 trunk: Guardrail for SELECT IN terms and their cartesian product

[GitBox <gi...@apache.org>]

ekaterinadimitrova2 commented on a change in pull request #1444:
URL: https://github.com/apache/cassandra/pull/1444#discussion_r817927185



##########
File path: src/java/org/apache/cassandra/config/GuardrailsOptions.java
##########
@@ -321,6 +322,31 @@ public void setReadBeforeWriteListOperationsEnabled(boolean enabled)
                                   x -> config.read_before_write_list_operations_enabled = x);
     }
 
+    @Override
+    public int getInSelectCartesianProductWarnThreshold()
+    {
+        return config.in_select_cartesian_product_warn_threshold;
+    }
+
+    @Override
+    public int getInSelectCartesianProductFailThreshold()
+    {
+        return config.in_select_cartesian_product_fail_threshold;
+    }
+
+    public void setInSelectCartesianProductThreshold(int warn, int fail)
+    {
+        validateIntThreshold(warn, fail, "in_select_cartesian_product");
+        updatePropertyWithLogging("in_select_cartesian_product_warn_threshold",
+                                  warn,
+                                  () -> config.in_select_cartesian_product_warn_threshold,
+                                  x -> config.in_select_cartesian_product_warn_threshold = x);
+        updatePropertyWithLogging("page_size_fail_threshold",

Review comment:
       ```suggestion
           updatePropertyWithLogging("in_select_cartesian_product_fail_threshold",
   ```
   I suspect copy paste issue? 

##########
File path: src/java/org/apache/cassandra/db/guardrails/Guardrails.java
##########
@@ -383,6 +397,23 @@ public int getPartitionKeysInSelectFailThreshold()
         return DEFAULT_CONFIG.getPartitionKeysInSelectFailThreshold();
     }
 

Review comment:
       ```suggestion
   
   @Override
   ```

##########
File path: src/java/org/apache/cassandra/cql3/restrictions/TokenRestriction.java
##########
@@ -205,7 +206,10 @@ protected PartitionKeyRestrictions doMergeWith(TokenRestriction otherRestriction
         @Override
         public List<ByteBuffer> bounds(Bound b, QueryOptions options) throws InvalidRequestException
         {
-            return values(options);
+            // ClientState is used by inSelectCartesianProduct guardrail to skip non-ordinary users.
+            // Passing null here to avoid polluting too many methods, because in case of EQ token restriction,
+            // it won't generate high cartesian product.
+            return values(options, null);

Review comment:
       So technically what you say is we don't skip here non-ordinary users by providing null but that is ok as we don't expect guardrails triggered? And it won't generate high Cartesian product because this is already guarded, right?

##########
File path: src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java
##########
@@ -221,6 +221,17 @@
      */
     void setReadBeforeWriteListOperationsEnabled(boolean enabled);
 
+    /**
+     * @return The threshold to warn when the number of partition keys in a select statement greater than threshold.
+     * -1 means disabled.
+     */
+    int getPartitionKeysInSelectWarnThreshold();

Review comment:
       Those two were missed before I guess?




-- 
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: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org