STAR-844 Read/Write hooks by jtgrabowski · Pull Request #250 · datastax/cassandra

jtgrabowski · 2021-09-14T14:33:08Z

This is a port of

https://github.com/riptano/bdp/commit/b6f0a18cb832c62f05cdcbd9cdcc2923f2fa727f
https://github.com/riptano/bdp/pull/19468
The first change set introduces QueryInfoTracker (QIT) interface and hooks it to StorageProxy. The second adds ClientState to the interface.

The original QIT utilizes ReadReconciliationObserver in the ReadTracker paths. Only onRow, onPartition and queried callbacks are utilized by CNDB and thus only these methods are ported to Converged Cassandra (CC). The callbacks are a bit different tho:

The callback methods are added directly to ReadTracker as CC doesn't have ReadReconciliationObserver. The class was added as a part of NodeSync effort and it is rather superfluous. Porting the whole class would add unnecessary complexity. Adding the required methods directly to the ReadTracker makes the interface cleaner and easier to understand.
CC operates on ReplicaPlans instead of plain host lists, that is why queried was changed to onReplicaPlan.

The PR adds empty QIT (with write callback only) + ClientState and then step by step extends it with more callback methods. It is advisable to review commit by commit although some of the code parts were slightly refactored along the way.

Other API parts were removed and will be added in the subsequent commits. Only the "write" API is present for now, it's implementation is added in the next commit.

mfleming · 2021-09-22T10:31:36Z

Just curious, how can this happen? I expected the lwtTacker to be the noop version at the very least?

good catch, it a leftover from a previous implementation. Fixed.

The ClientState conveys information about the client. It will be passed to trackers in subsequent commits. Tracker implementations are free to interpret client's information supplied to StorageProxy. For the most cases the ClientState instance comes directly from QueryState but in case of Tracing on replicas this approach in impossible. Tracing on replicas reconstructs TracingState from Message.Header. ClientState is mocked there from ClientState.forInternalCalls with traced keyspace. The keyspace has been added to the Message.Header for this purpose only.

Writes and batch writes are the simplest case for tracking. The trackers watch onf for write start and end (success or failure).

So far we do not count transferred rows/partitions only number of [range] reads.

So far we do not count transferred rows/partitions only number of read/writes.

Row and Partition tracking is achieved by `ReadTrackingTransformation` applied to unfiltered row iterators. We apply the transformation early to prepare for possible tracker extension with tombstone accounting (that is present in the original QIT).

Don't track the first phase of RFP to avoid extra calls to ReadTracker callback methods. Only the "final" unfiltered rows are tracked.

This substitutes `queried` callback method from the original QIT. OSS C* uses ReplicaPlans instead of plain endpoint lists.

this caused confusion when QIT test was executed with other tests.

In the original patch traced keyspace comes from TracingClientState, not from ClientState. This commit fixes this mistake. Additionally add graceful handling nulls for traced keyspace as adding null values to trace headers results in NPE.

blambov · 2021-09-23T08:36:45Z

+ * <p>For writes, the {@link #onWrite} method is only called for the "user write", but if that write trigger either
+ * secondary index or materialized views updates, those additional update do not trigger additional calls.
+ *
+ * <p>The methods of this tracker are called on hot path, so none of them should be blocking, and they should be as


The "blocking" part is a TPC artifact, there's no need for implementations to be non-blocking here.

blambov · 2021-09-23T08:39:32Z

+     */
+    interface ReadTracker extends Tracker
+    {
+        ReadTracker NOOP = new ReadTracker()


Nit: although these are constants, I find things much easier to read if they are placed after the type is actually defined i.e. at the end rather than at the beginning of the interface/class.

blambov · 2021-09-23T08:45:23Z

            metrics.casWriteMetrics.timeouts.mark();
            metrics.writeMetricsMap.get(consistencyForPaxos).timeouts.mark();
+            lwtTracker.onError(wte);
            throw new CasWriteTimeoutException(wte.writeType, wte.consistency, wte.received, wte.blockFor, wte.contentions);


Curious why new exception with the same data rather than just throw wte? This is obfuscating the stack trace of the actual failure.

true, I don't know why tho.

blambov · 2021-09-23T08:48:18Z

+                                                      consistencyLevel,
+                                                      queryState,
+                                                      queryStartNanoTime,
+                                                      readTracker), command);


Nit: command should be on a new line.

blambov · 2021-09-23T08:51:10Z


-        return StorageProxy.read(Group.one(this), consistency, queryState, queryStartNanoTime);
+        Group group = Group.one(this);
+        QueryInfoTracker.ReadTracker readTracker = StorageProxy.queryTracker().onRead(queryState.getClientState(),


It looks like this is not the right place for the onRead, onDone pair. Tracking is said to be StorageProxy-level, and this will catch more calls than the ones going through that. It is also inconsistent with the range query path.

I moved it down to StorageProxy. This doesn't change the logic, though.

blambov · 2021-09-23T08:59:59Z

         */

        UnfilteredPartitionIterator merged = UnfilteredPartitionIterators.merge(results, mergeListener);
+        UnfilteredPartitionIterator trackedPartitionIterator = Transformation.apply(merged, new ReadTrackingTransformation(resolveReadTracker));


This has some cost to performance; can we make transformation conditional on there being a tracker or alternatively verify that the cost is negligible?

changed for DataResolver and DigestResolver

jtgrabowski · 2021-09-23T11:56:33Z

@blambov I applied your remarks in the latest commit 8f9cba2.

blambov

LGTM

sonarqubecloud · 2021-09-23T12:39:36Z

Kudos, SonarCloud Quality Gate passed!

1 Bug
0 Vulnerabilities
0 Security Hotspots
21 Code Smells

79.0% Coverage
0.0% Duplication

This is a port of - https://github.com/riptano/bdp/commit/b6f0a18cb832c62f05cdcbd9cdcc2923f2fa727f - https://github.com/riptano/bdp/pull/19468 The first change set introduces QueryInfoTracker (QIT) interface and hooks it to StorageProxy. The second adds ClientState to the interface. The original QIT utilizes ReadReconciliationObserver in the ReadTracker paths. Only onRow, onPartition and queried callbacks are utilized by CNDB and thus only these methods are ported to Converged Cassandra (CC). The callbacks are a bit different tho: - The callback methods are added directly to ReadTracker as CC doesn't have ReadReconciliationObserver. The class was added as a part of NodeSync effort and it is rather superfluous. Porting the whole class would add unnecessary complexity. Adding the required methods directly to the ReadTracker makes the interface cleaner and easier to understand. - CC operates on ReplicaPlans instead of plain host lists, that is why queried was changed to onReplicaPlan. (cherry picked from commit c32e91f) (cherry picked from commit 093bc63)

This is a port of - https://github.com/riptano/bdp/commit/b6f0a18cb832c62f05cdcbd9cdcc2923f2fa727f - https://github.com/riptano/bdp/pull/19468 The first change set introduces QueryInfoTracker (QIT) interface and hooks it to StorageProxy. The second adds ClientState to the interface. The original QIT utilizes ReadReconciliationObserver in the ReadTracker paths. Only onRow, onPartition and queried callbacks are utilized by CNDB and thus only these methods are ported to Converged Cassandra (CC). The callbacks are a bit different tho: - The callback methods are added directly to ReadTracker as CC doesn't have ReadReconciliationObserver. The class was added as a part of NodeSync effort and it is rather superfluous. Porting the whole class would add unnecessary complexity. Adding the required methods directly to the ReadTracker makes the interface cleaner and easier to understand. - CC operates on ReplicaPlans instead of plain host lists, that is why queried was changed to onReplicaPlan. (cherry picked from commit c32e91f) (cherry picked from commit 093bc63) (cherry picked from commit efdfa1f)

This is a port of - https://github.com/riptano/bdp/commit/b6f0a18cb832c62f05cdcbd9cdcc2923f2fa727f - https://github.com/riptano/bdp/pull/19468 The first change set introduces QueryInfoTracker (QIT) interface and hooks it to StorageProxy. The second adds ClientState to the interface. The original QIT utilizes ReadReconciliationObserver in the ReadTracker paths. Only onRow, onPartition and queried callbacks are utilized by CNDB and thus only these methods are ported to Converged Cassandra (CC). The callbacks are a bit different tho: - The callback methods are added directly to ReadTracker as CC doesn't have ReadReconciliationObserver. The class was added as a part of NodeSync effort and it is rather superfluous. Porting the whole class would add unnecessary complexity. Adding the required methods directly to the ReadTracker makes the interface cleaner and easier to understand. - CC operates on ReplicaPlans instead of plain host lists, that is why queried was changed to onReplicaPlan. (cherry picked from commit c32e91f) (cherry picked from commit 093bc63) (cherry picked from commit efdfa1f) (cherry picked from commit 2ab888d)

This is a port of - riptano/bdp@b6f0a18 - riptano/bdp#19468 The first change set introduces QueryInfoTracker (QIT) interface and hooks it to StorageProxy. The second adds ClientState to the interface. The original QIT utilizes ReadReconciliationObserver in the ReadTracker paths. Only onRow, onPartition and queried callbacks are utilized by CNDB and thus only these methods are ported to Converged Cassandra (CC). The callbacks are a bit different tho: - The callback methods are added directly to ReadTracker as CC doesn't have ReadReconciliationObserver. The class was added as a part of NodeSync effort and it is rather superfluous. Porting the whole class would add unnecessary complexity. Adding the required methods directly to the ReadTracker makes the interface cleaner and easier to understand. - CC operates on ReplicaPlans instead of plain host lists, that is why queried was changed to onReplicaPlan. (cherry picked from commit c32e91f) (cherry picked from commit 093bc63) (cherry picked from commit efdfa1f) (cherry picked from commit 2ab888d)

This is a port of - riptano/bdp@b6f0a18 - riptano/bdp#19468 The first change set introduces QueryInfoTracker (QIT) interface and hooks it to StorageProxy. The second adds ClientState to the interface. The original QIT utilizes ReadReconciliationObserver in the ReadTracker paths. Only onRow, onPartition and queried callbacks are utilized by CNDB and thus only these methods are ported to Converged Cassandra (CC). The callbacks are a bit different tho: - The callback methods are added directly to ReadTracker as CC doesn't have ReadReconciliationObserver. The class was added as a part of NodeSync effort and it is rather superfluous. Porting the whole class would add unnecessary complexity. Adding the required methods directly to the ReadTracker makes the interface cleaner and easier to understand. - CC operates on ReplicaPlans instead of plain host lists, that is why queried was changed to onReplicaPlan. (cherry picked from commit c32e91f) (cherry picked from commit 093bc63) (cherry picked from commit efdfa1f) (cherry picked from commit 2ab888d) (cherry picked from commit 546bc4a)

This is a port of - riptano/bdp@b6f0a18 - riptano/bdp#19468 The first change set introduces QueryInfoTracker (QIT) interface and hooks it to StorageProxy. The second adds ClientState to the interface. The original QIT utilizes ReadReconciliationObserver in the ReadTracker paths. Only onRow, onPartition and queried callbacks are utilized by CNDB and thus only these methods are ported to Converged Cassandra (CC). The callbacks are a bit different tho: - The callback methods are added directly to ReadTracker as CC doesn't have ReadReconciliationObserver. The class was added as a part of NodeSync effort and it is rather superfluous. Porting the whole class would add unnecessary complexity. Adding the required methods directly to the ReadTracker makes the interface cleaner and easier to understand. - CC operates on ReplicaPlans instead of plain host lists, that is why queried was changed to onReplicaPlan. (cherry picked from commit c32e91f) (cherry picked from commit 093bc63) (cherry picked from commit efdfa1f) (cherry picked from commit 2ab888d) (cherry picked from commit 546bc4a) STAR-844 Fix rebase conflict in ParamType with CASSANDRA-17981 (cherry picked from commit 12cd994)

This is a port of - https://github.com/riptano/bdp/commit/b6f0a18cb832c62f05cdcbd9cdcc2923f2fa727f - https://github.com/riptano/bdp/pull/19468 The first change set introduces QueryInfoTracker (QIT) interface and hooks it to StorageProxy. The second adds ClientState to the interface. The original QIT utilizes ReadReconciliationObserver in the ReadTracker paths. Only onRow, onPartition and queried callbacks are utilized by CNDB and thus only these methods are ported to Converged Cassandra (CC). The callbacks are a bit different tho: - The callback methods are added directly to ReadTracker as CC doesn't have ReadReconciliationObserver. The class was added as a part of NodeSync effort and it is rather superfluous. Porting the whole class would add unnecessary complexity. Adding the required methods directly to the ReadTracker makes the interface cleaner and easier to understand. - CC operates on ReplicaPlans instead of plain host lists, that is why queried was changed to onReplicaPlan. (cherry picked from commit c32e91f) (cherry picked from commit 093bc63) (cherry picked from commit efdfa1f) (cherry picked from commit 2ab888d) (cherry picked from commit 546bc4a) STAR-844 Fix rebase conflict in ParamType with CASSANDRA-17981 (cherry picked from commit 12cd994) (cherry picked from commit 5ce23cf)

This is a port of - riptano/bdp@b6f0a18 - riptano/bdp#19468 The first change set introduces QueryInfoTracker (QIT) interface and hooks it to StorageProxy. The second adds ClientState to the interface. The original QIT utilizes ReadReconciliationObserver in the ReadTracker paths. Only onRow, onPartition and queried callbacks are utilized by CNDB and thus only these methods are ported to Converged Cassandra (CC). The callbacks are a bit different tho: - The callback methods are added directly to ReadTracker as CC doesn't have ReadReconciliationObserver. The class was added as a part of NodeSync effort and it is rather superfluous. Porting the whole class would add unnecessary complexity. Adding the required methods directly to the ReadTracker makes the interface cleaner and easier to understand. - CC operates on ReplicaPlans instead of plain host lists, that is why queried was changed to onReplicaPlan. (cherry picked from commit c32e91f) (cherry picked from commit 093bc63) (cherry picked from commit efdfa1f) (cherry picked from commit 2ab888d) (cherry picked from commit 546bc4a) STAR-844 Fix rebase conflict in ParamType with CASSANDRA-17981 (cherry picked from commit 12cd994) (cherry picked from commit 5ce23cf)

This is a port of - riptano/bdp@b6f0a18 - riptano/bdp#19468 The first change set introduces QueryInfoTracker (QIT) interface and hooks it to StorageProxy. The second adds ClientState to the interface. The original QIT utilizes ReadReconciliationObserver in the ReadTracker paths. Only onRow, onPartition and queried callbacks are utilized by CNDB and thus only these methods are ported to Converged Cassandra (CC). The callbacks are a bit different tho: - The callback methods are added directly to ReadTracker as CC doesn't have ReadReconciliationObserver. The class was added as a part of NodeSync effort and it is rather superfluous. Porting the whole class would add unnecessary complexity. Adding the required methods directly to the ReadTracker makes the interface cleaner and easier to understand. - CC operates on ReplicaPlans instead of plain host lists, that is why queried was changed to onReplicaPlan. (cherry picked from commit c32e91f) (cherry picked from commit 093bc63) (cherry picked from commit efdfa1f) (cherry picked from commit 2ab888d) (cherry picked from commit 546bc4a) STAR-844 Fix rebase conflict in ParamType with CASSANDRA-17981 (cherry picked from commit 12cd994) (cherry picked from commit 5ce23cf) (Rebase of commit 99dbcaa)

jtgrabowski force-pushed the STAR-844 branch 3 times, most recently from 5745947 to 8b46c7b Compare September 21, 2021 14:23

jtgrabowski marked this pull request as ready for review September 21, 2021 18:26

jtgrabowski force-pushed the STAR-844 branch from 8b46c7b to 46e5216 Compare September 21, 2021 18:38

STAR-844 port QIT interface with writes

0f8fce7

Other API parts were removed and will be added in the subsequent commits. Only the "write" API is present for now, it's implementation is added in the next commit.

jtgrabowski force-pushed the STAR-844 branch from 46e5216 to bb77585 Compare September 22, 2021 08:30

jtgrabowski requested review from blambov and mfleming September 22, 2021 09:11

jtgrabowski force-pushed the STAR-844 branch from bb77585 to 90f34ef Compare September 22, 2021 09:52

mfleming approved these changes Sep 22, 2021

View reviewed changes

jtgrabowski added 10 commits September 22, 2021 15:03

STAR-844 add write and batch write tracking

dada0e5

Writes and batch writes are the simplest case for tracking. The trackers watch onf for write start and end (success or failure).

STAR-844 add read and range read tracking

602e65e

So far we do not count transferred rows/partitions only number of [range] reads.

STAR-844 add LWT read/write tracking

11d05f7

So far we do not count transferred rows/partitions only number of read/writes.

STAR-844 add tests for error counting

7734dc2

STAR-844 add RFP test

30b54ce

Don't track the first phase of RFP to avoid extra calls to ReadTracker callback methods. Only the "final" unfiltered rows are tracked.

STAR-844 add replica plan tracking

df119cf

This substitutes `queried` callback method from the original QIT. OSS C* uses ReplicaPlans instead of plain endpoint lists.

STAR-844 review remarks

bc46b51

STAR-844 don't remove CqlTester KEYSPACE between runs

8bce859

this caused confusion when QIT test was executed with other tests.

jtgrabowski force-pushed the STAR-844 branch from 90f34ef to 8bce859 Compare September 22, 2021 13:04

STAR-844 fix tracing broken tests

dd8a51c

In the original patch traced keyspace comes from TracingClientState, not from ClientState. This commit fixes this mistake. Additionally add graceful handling nulls for traced keyspace as adding null values to trace headers results in NPE.

jtgrabowski force-pushed the STAR-844 branch from 946645d to dd8a51c Compare September 23, 2021 08:21

blambov reviewed Sep 23, 2021

View reviewed changes

STAR-844 review remarks

8f9cba2

blambov approved these changes Sep 23, 2021

View reviewed changes

jtgrabowski merged commit c32e91f into ds-trunk Sep 23, 2021

Conversation

jtgrabowski commented Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtgrabowski commented Sep 23, 2021

Uh oh!

blambov left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Sep 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jtgrabowski commented Sep 14, 2021 •

edited

Loading