You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/03/27 06:42:19 UTC
[GitHub] [cassandra] sathyakplm opened a new pull request #1528: CASSANDRA-17458 Fix timing issue in testPartitionDeletionRangeDeletionTie test case
sathyakplm opened a new pull request #1528:
URL: https://github.com/apache/cassandra/pull/1528
#### The Problem
There are two delete statements in this test case.
##### First delete statement
```
QueryProcessor.executeOnceInternalWithNowAndTimestamp(nowInSec,
timestamp,
"DELETE FROM ks.partition_range_deletion USING TIMESTAMP 10 WHERE k=1");
```
##### Second delete statement
```
QueryProcessor.executeOnceInternalWithNowAndTimestamp(nowInSec,
timestamp,
"DELETE FROM ks.partition_range_deletion USING TIMESTAMP 10 WHERE k=1 and c1=1");
```
The test case fails when the first statement and second statement falls in different `second` boundaries. We are passing `nowInSec`; but, this is not being respected while the entry is inserted. `FBUtilities.nowInSeconds()` is getting called instead.
#### Solution
Use `makeInternalOptionsWithNowInSec` instead of `makeInternalOptions`. This change makes sure that it respects the variable `nowInSec`.
--
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
[GitHub] [cassandra] blerer commented on a change in pull request #1528: CASSANDRA-17458 Fix timing issue in testPartitionDeletionRangeDeletionTie test case
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #1528:
URL: https://github.com/apache/cassandra/pull/1528#discussion_r836556133
##########
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:
Thanks for the explanation. I was confused because the logic was not the one that I remembered. Apparently, it has been changed recently. Let me dig in. :-)
--
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
[GitHub] [cassandra] blerer commented on a change in pull request #1528: CASSANDRA-17458 Fix timing issue in testPartitionDeletionRangeDeletionTie test case
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #1528:
URL: https://github.com/apache/cassandra/pull/1528#discussion_r836360872
##########
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:
I am a bit confused with the change. I feel that the change should be at the test level not at the QueryProcessor level.
Could you elaborate on why you believe that we should modify the code here?
--
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
[GitHub] [cassandra] sathyakplm commented on a change in pull request #1528: CASSANDRA-17458 Fix timing issue in testPartitionDeletionRangeDeletionTie test case
Posted by 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