[GitHub] [cassandra] sathyakplm commented on a change in pull request #1528: CASSANDRA-17458 Fix timing issue in testPartitionDeletionRangeDeletionTie test case

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

sathyakplm commented on a change in pull request #1528:
URL: https://github.com/apache/cassandra/pull/1528#discussion_r836426248



##########
File path: src/java/org/apache/cassandra/cql3/QueryProcessor.java
##########
@@ -575,7 +575,7 @@ private static UntypedResultSet executeOnceInternal(QueryState queryState, Strin
     {
         CQLStatement statement = parseStatement(query, queryState.getClientState());
         statement.validate(queryState.getClientState());
-        ResultMessage result = statement.executeLocally(queryState, makeInternalOptions(statement, values));
+        ResultMessage result = statement.executeLocally(queryState, makeInternalOptionsWithNowInSec(statement, queryState.getNowInSeconds(), values));

Review comment:
       Let's look at this particular call hierarchy when `executeOnceInternalWithNowAndTimestamp` is called.
   
   ```
   1 -> QueryProcessor :: public static UntypedResultSet executeOnceInternalWithNowAndTimestamp(int nowInSec, long timestamp, String query, Object... values)
   2 --> QueryProcessor :: private static UntypedResultSet executeOnceInternal(QueryState queryState, String query, Object... values)
   3 ---> QueryProcessor :: public static QueryOptions makeInternalOptions(CQLStatement prepared, Object[] values)
   4 ----> QueryProcessor :: private static QueryOptions makeInternalOptions(CQLStatement prepared, Object[] values, ConsistencyLevel cl)
   5 -----> QueryProcessor :: private static QueryOptions makeInternalOptionsWithNowInSec(CQLStatement prepared, int nowInSec, Object[] values, ConsistencyLevel cl)
   ```
   
   Look at how the call goes from step 4 to 5 in the above call hierarchy. The actual code how step 4 calls step 5 is below:
   ```
   return makeInternalOptionsWithNowInSec(prepared, FBUtilities.nowInSeconds(), values, cl);
   ```
   
   At this point, the `nowInSec` passed to `executeOnceInternalWithNowAndTimestamp` is not used; instead `FBUtilities.nowInSeconds()` is being used in the `SpecificOptions`.
   
   Now, let's see how this `SpecificOptions` is being used while writing the data. Let's look at the following hierarchy:
   
   ```
   1 -> QueryProcessor :: public static UntypedResultSet executeOnceInternalWithNowAndTimestamp(int nowInSec, long timestamp, String query, Object... values)
   2 --> QueryProcessor :: private static UntypedResultSet executeOnceInternal(QueryState queryState, String query, Object... values)
   3 ---> ModificationStatement :: public ResultMessage executeLocally(QueryState queryState, QueryOptions options) throws RequestValidationException, RequestExecutionException
   4 ----> ModificationStatement :: public ResultMessage executeInternalWithCondition(QueryState state, QueryOptions options)
   5 -----> QueryOptions :: public int getNowInSeconds(QueryState state)
   ```
   In the last step (5), we can see that the now in seconds is fetched using specific options: `int nowInSeconds = getSpecificOptions().nowInSeconds;`
   
   The `nowInSec` obtained from the specific options is being used to apply the mutation:
   ```
   for (IMutation mutation : getMutations(queryState.getClientState(), options, true, timestamp, nowInSeconds, queryStartNanoTime))
               mutation.apply();
   ```
   
   Summary:  `nowInSec` that is being used to apply the mutation is not the one we pass in `executeOnceInternalWithNowAndTimestamp`; but, the one which gets generated in`ModificationStatement :: public ResultMessage executeInternalWithCondition(QueryState state, QueryOptions `.
   
   Now, let's look at when the test case fails.
   
   The `nowInSec` generated in `QueryProcessor.java:384` is being used to write the data. It doesn't matter what we pass to `executeOnceInternalWithNowAndTimestamp`. Let me paste few lines from the test case to illustrate the problem.
   
   ```
               final long timestamp = FBUtilities.timestampMicros();
               final int nowInSec = FBUtilities.nowInSeconds();
   
               QueryProcessor.executeOnceInternalWithNowAndTimestamp(nowInSec,
                                                                     timestamp,
                                                                     "DELETE FROM ks.partition_range_deletion USING TIMESTAMP 10 WHERE k=1");
               if (flush && multiSSTable)
                   cfs.forceBlockingFlush();
               QueryProcessor.executeOnceInternalWithNowAndTimestamp(nowInSec,
                                                                     timestamp,
                                                                     "DELETE FROM ks.partition_range_deletion USING TIMESTAMP 10 WHERE k=1 and c1=1");
   
   ``` 
   There are two delete statements. The `nowInSec` that we pass along with the statements does not matter. Everything works fine as long as the first and second statement are in same same second `S`. The test fails when the first statement is in `S` and the second statement execute in `S+1`.
   
   Below are my assumptions why this breaks when these two delete statements are in two different second intervals. Please correct me if I am wrong.
   
   If `TIMESTAMP == 10` and `timeInSec == S` for both the statements, both the delete statements are accounted under `TIMESTAMP 10`.
   
   If `TIMESTAMP == 10` and `timeInSec == S` for first statement and `TIMESTAMP == 10` and `timeInSec == S+1` for second statement, the first statement is accounted for `TIMESTAMP 10` and the second statement is accounted for a `TIMESTAMP > 10`.
   
   @blerer , let me know if this explains. If there are other ways to solve this issue, I would be happy to learn.




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