Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CNDB-8501 Propagate READ_BYTES/WRITE_BYTES sensor via native CQL custom payload #1295

Open
wants to merge 48 commits into
base: main
Choose a base branch
from

Conversation

aymkhalil
Copy link

@aymkhalil aymkhalil commented Sep 21, 2024

Addresses: https://github.com/riptano/cndb/issues/8501
CNDB PR: https://github.com/riptano/cndb/pull/11681

The idea is to expose sensors files to CQL client via the native protocol's custom payload (which is available in V4 or higher). This first iteration add READ_BYTES and WRITE_BYTES.

@aymkhalil aymkhalil requested a review from sbtourist September 21, 2024 04:17
@aymkhalil aymkhalil changed the title CNDB-8501 Propagate sensors vials via native CQL custom payload CNDB-8501 Propagate sensors via native CQL custom payload Sep 24, 2024
@aymkhalil aymkhalil requested a review from maxtomassi October 1, 2024 23:19
@aymkhalil aymkhalil marked this pull request as ready for review October 2, 2024 05:34
@aymkhalil aymkhalil changed the title CNDB-8501 Propagate sensors via native CQL custom payload CNDB-8501 Propagate READ_BYTES sensor via native CQL custom payload Oct 2, 2024
Copy link
Author

@aymkhalil aymkhalil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ee9566e has the following changes:

  • Increment sensors for each replica (as opposed to getting sensors value from one and multiplying by replica count). This means we guarantee at least a quorum of replicas sensors rolled up, up to replica count
  • Add replica responses for writes in memory so later on they can be consulted to rollup sensors values on the REQUEST_RESPONSE stage
  • Add a RequestSensorsFactory#requestSensorSuffixSupplier to customize sensors prefix in CNDB. Mainly used to append <Keyspace without tenant>.<table name> to WRITE_BYTES_REQUEST header to support batch operations.

@aymkhalil aymkhalil changed the title CNDB-8501 Propagate READ_BYTES sensor via native CQL custom payload CNDB-8501 Propagate READ_BYTES/WRITE_BYTES sensor via native CQL custom payload Oct 15, 2024
*
* @return a function that encodes a sensor to a string
*/
default Function<Sensor, String> requestSensorEncoder()
Copy link
Author

@aymkhalil aymkhalil Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we could combine requestSensorEncoder & registrySensorEncoder into one by either:

  • Function<Sensor, String> createSensorEncoder(Scope scope) where scope is an Enum of {REQUEST, TABLE}.
  • Create a separate interface SensorEncoder with an encode method that accepts both Sensor and Scope

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create a separate interface SensorEncoder with an encode method that accepts both Sensor and Scope

I think a dedicated interface would make things clearer, though I would use two different methods rather than an enum?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

though I would use two different methods rather than an enum

Makes sense. I think at most we'll have two methods. It is not like it will keep growing with time so an enum (which is basically reduced to a binary check) is not warranted.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

@sbtourist sbtourist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First review pass done (no tests).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class doesn't cover batches, is that something you plan to add?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference is not to add it in this patch as it is getting bigger in scope. However, this patch should make it easy to add in terms of reusable methods to add sensors & sensor encoder override in CNDB.

I plan to capture all remaining use cases/sensors in one issue after this one is merged.

Copy link

@sbtourist sbtourist Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on the fence here: it's branded as "Propagate READ_BYTES/WRITE_BYTES sensor via native CQL custom payload", and it does that for some statements, but not all.

Do you have any sense of how much bigger would the patch get?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean, what I had in mind is other types like Counter Mutation/Index Writer/internode. But should cover the statements that feed into read/write sensors. I'll add them.

src/java/org/apache/cassandra/net/ResponseVerbHandler.java Outdated Show resolved Hide resolved
src/java/org/apache/cassandra/net/SensorsCustomParams.java Outdated Show resolved Hide resolved
src/java/org/apache/cassandra/net/SensorsCustomParams.java Outdated Show resolved Hide resolved
@@ -1061,6 +1071,12 @@ public static void mutate(List<? extends IMutation> mutations,

QueryInfoTracker.WriteTracker writeTracker = queryTracker().onWrite(state, false, mutations, consistencyLevel);

// Request sensors are utilized to track usages from replicas serving a write request
RequestSensors sensors = CassandraRelevantProperties.PROPAGATE_REQUEST_SENSORS_VIA_NATIVE_PROTOCOL.getBoolean() ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this use RequestSensorsFactory rather than creating things manually? Likewise for reads below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this:

  • Unlike on writers, here what decides if we should track sensors is the REQUEST_SENSORS_VIA_NATIVE_PROTOCOL flag & whether or not sensors values exit in writer response. From this point on, ActiveRequestSensors is the right choice.
  • a write request at the storage proxy level could contain mutations that belong to different keyspaces. Because we have one RequestSensors per thread, I wasn't sure how to make it per ks. It is worth noting that as far as CNDB is concerned, we enable/disable per tenant (which all keyspaces will belong to a single tenant)

Thoughts?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also using RequestSensorsFactory would effectively mean (at least in CNDB case) running the logic RequestSensorsFactory#create(keyspace) twice per request - once on the coordinator and another on the writer.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike on writers, here what decides if we should track sensors is the REQUEST_SENSORS_VIA_NATIVE_PROTOCOL flag & whether or not sensors values exit in writer response.

I'm not sure I understand this correctly, but I'd say at this level we should always use RequestSensorsFactory as we're deciding how to track sensors, then we should use REQUEST_SENSORS_VIA_NATIVE_PROTOCOL when deciding if returning them on the native protocol response. Makes sense?

a write request at the storage proxy level could contain mutations that belong to different keyspaces. Because we have one RequestSensors per thread, I wasn't sure how to make it per ks

Good point. This is kind of an anomaly by the API point of view. The instinct says "it's an anomaly, so ignore it and assumes all keyspaces are enabled (or not)", but let me think about that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand this correctly, but I'd say at this level we should always use RequestSensorsFactory as we're deciding how to track sensors, then we should use REQUEST_SENSORS_VIA_NATIVE_PROTOCOL when deciding if returning them on the native protocol response. Makes sense?

I see what you mean. What I'm trying to say is the decision criteria to track sensors on the writer is RequestSensorsFactory but here it is REQUEST_SENSORS_VIA_NATIVE_PROTOCOL. If we disable the latter, there is no point to track sensors on coordinator (at least as far as this patch is concerned). Also, because we have a single RequestSensorsFactory the track/don't track decision will always be the same on writer and coordinators and it sounded redundant to rerun the logic on both components.

Having said that, I don't have a strong opinion here, and the bigger issue is the decision RequestSensorsFactory per ks discussion point so I'll put this on hold until we conclude on the other one.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually I'm convinced - lets figure out the multiple keyspace things and I'll use RequestSensorsFactory across the board.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: f9c7af5

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CAS, batches and range reads seem not to be covered: is that inteded?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let us first conclude on the simple read/write case and then I'll create issue/add the rest in a separate patch.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my other related comment.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added for CAS: d39b40a

Rest are coming up...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added for batch: bd952b4

Copy link
Author

@aymkhalil aymkhalil Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added for range: dfafba6 (also refactored SensorsTest in a separate commit b9b4f53)

if (sensors == null)
return;

if (callbackInfo instanceof RequestCallbacks.WriteCallbackInfo)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, rather than retrieving the sensors and then populating them here, which requires an instanceof, we should add a populateRequestSensors() method directly to the callback and have specific implementations in there.

That said, AFAIU you've made this because we only have access to the mutation in WriteCallbackInfo, where:

this.mutation = shouldHint(allowHints, message, consistencyLevel) ? message.payload : null;

So the mutation could actually be null.

Two options here:

  1. Make sure the mutation is never null in WriteCallbackInfo.
  2. Rework things so that the mutation can be accessed from the callback.

Thoughts?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIU you've made this because we only have access to the mutation in WriteCallbackInfo

Yes, callbacks (aka. response handlers) are decoupled from the Mutation (after sending Mutation to replicas there is no link back - see where they part ways)

Make sure the mutation is never null in WriteCallbackInfo.

So WriteCallbackInfo extends CallbackInfo - I could store a non-nullable request message payload on the callback info itself which has the following downsides:

  • WriteCallbackInfo would still have the nullable mutation to avoid changing the shouldHint behavior. Which is a bit weird if the same variable is introduced in parent class.
  • Would be redundant for ReadCallback as this callback already has the payload (which is ReadCommand). In other words, callbackInfo.payload == callbackInfo.callback.payload for this case.
  • In any case, we'd still need an instanceof (which btw I agree is underwhelming) to construct a sensors context (Context.from(ReadCommand) vs Context.from(Mutation))

Rework things so that the mutation can be accessed from the callback.

This would be ideal. I tried something like a final instance Mutation variable on AbstractWriteResponseHandler but the issue is we have many different classes implementing AbstractWriteResponseHandler, including in CNDB, and some using composition over inheritance leaving lots of constructors to override. Also a Mutator interface with different implementations to instantiate the callback that we should pass the Mutation to. Perhaps if we end up deciding on this one, I'll do it in separate patch otherwise this one will be more disturbing to review.

Thoughts on the thoughts?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you convinced me to give up and leave things as they are :)

@@ -338,7 +342,7 @@ static class WriteCallbackInfo extends CallbackInfo

public boolean shouldHint()
{
return mutation != null && StorageProxy.shouldHint(replica);
return shouldHint && StorageProxy.shouldHint(replica);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (in addition to `getIMutation on line 362) is the "safest workaround" I could make to carry around a non-nullable Mutation object.

I dunno the history here but I think mutation nullability was dependent on shouldHint is to be memory savvy. For Sensors we need this object at all times. We could add another object or just modify the behavior here to be shouldHint independent. I opted for the latter because judging by tests that failed (on C* & CNDB) majority of use cases use shouldHint so instead of introducing another mutation object for sensors (Which would be redundant most of the time) I thought to decouple mutation nullability from shouldHint

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dunno the history here but I think mutation nullability was dependent on shouldHint is to be memory savvy.

Yep most probably. Looks good.

* Used for sensors tracking. A safe alternative to {@link #mutation()} to access counter mutations without
* changing existing assert behavior.
*/
public IMutation getIMutation()
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks weird, but the existing mutation() method returns Mutation which is convenient for hints. In order not to changes the assertion behavior and refactoring by returning IMutation - I opted in for a different getter. Note that CounterMutation is IMutation but not Mutation and is needed for sensors (but not hints)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: for consistency I'd call this iMutation().

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/java/org/apache/cassandra/sensors/SensorsFactory.java Outdated Show resolved Hide resolved
src/java/org/apache/cassandra/sensors/SensorsFactory.java Outdated Show resolved Hide resolved
src/java/org/apache/cassandra/sensors/SensorsFactory.java Outdated Show resolved Hide resolved
src/java/org/apache/cassandra/sensors/SensorsFactory.java Outdated Show resolved Hide resolved
SensorEncoder DEFAULT_SENSOR_ENCODER = new SensorEncoder()
{
@Override
public String encodeRequestSensor(Sensor sensor)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make these two methods return an Optional string, which would be empty by default, so that callers will actually encode and propagate only ifPresent?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me, will apply together with the other comment regarding interface method names

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

98c0731

although I haven't necessarily used ifPresent because we have typical have another optional for Sensor at encoding time and nested ifPresent deemed less readable. I opted in for early returns. I do see the benefit of not encoding if empty though (better than returning empty strings)

src/java/org/apache/cassandra/service/StorageProxy.java Outdated Show resolved Hide resolved
assertThat(onResponseStartSignal[replica].await(1, TimeUnit.SECONDS)).isTrue();
}

private void testPrepareCallbackSensorsTracking(Message writeRequest, boolean allowHints) throws InterruptedException

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like an odd name?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was trying to communicate the fact that the test dimensions are generally the callback type used (rather than the verb or the Message type, with few exceptions :/) because this is how sensors are tracked in ResponseVerbHandler) but a better name is warranted.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several misspellings of actual (i.e. acutal).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Parameterized.Parameters(name = "schema={0}, prepQueries={1}, testQuery={2}, expectedHeaders={3}")
public static Collection<Object[]> data()
{
String tableSchema = withKeyspace("CREATE TABLE %s.tbl (pk int PRIMARY KEY, v1 text)");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two tables have the same name, is this actually working?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this actually working?

Yes. If it isn't cassci-bot will report a failure as distributed tests are run in CI. Needless to say it works locally.

All tables are created/destroyed per test permutation, it just happens this one has a different schema.

The reason tables are created in each test permutation is to isolate sensors (won't make much sense for example to read -> assert READ_BYTES -> range read --> assert READ_BYTES unless we opt in for testing exact increments of sensor values which is not really the intent of this test).

return new ResultMessage.Void();
ResultMessage<ResultMessage.Void> result = new ResultMessage.Void();
RequestSensors sensors = RequestTracker.instance.get();
for (ModificationStatement statement : statements)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all statements refer to the same context, are we then adding the same sensor over and over again?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch. It is suboptimal (but benign, as addSensorToCQLResponse will overwrite the sensor in the header with the same value again and again) - fixed: b752ae8

Copy link
Author

@aymkhalil aymkhalil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed feedback from 11/27

/**
* If true, the coordinator will propagate request sensors via the native protocol custom payload bytes map.
*/
REQUEST_SENSORS_VIA_NATIVE_PROTOCOL("cassandra.request_sensors_via_native_protocol", "false");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return new ResultMessage.Void();
ResultMessage<ResultMessage.Void> result = new ResultMessage.Void();
RequestSensors sensors = RequestTracker.instance.get();
for (ModificationStatement statement : statements)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch. It is suboptimal (but benign, as addSensorToCQLResponse will overwrite the sensor in the header with the same value again and again) - fixed: b752ae8

{
ResultMessage result = super.execute(state, options, queryStartNanoTime);

if (result == null) result = new ResultMessage.Void();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

private static void addSensorsToResponse(Collection<Sensor> sensors,
Function<String, String> requestParamSupplier,
Function<String, String> tableParamSupplier,
Message.Builder<NoPayload> response,
int replicaMultiplier)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 118d27c


/**
* Provides a factory to customize the behaviour of sensors tracking in CNDB by providing to factory methods:
* <li> {@link SensorsFactory#createRequestSensors} provides {@link RequestSensors} implementation to active or deactivate sensors per keyspace</li>
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed along side other SensorsFactory java docs comments: 425d27b


/**
* A utility class that groups methods to facilitate encoding sensors in native or internode protocol messages:
* <li>Sensors in internode messages: Use to communicate sensors values from replicas to coordinators in the internode
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed along side other java docs comments in SensorsCustomParams be77507

@Parameterized.Parameters(name = "schema={0}, prepQueries={1}, testQuery={2}, expectedHeaders={3}")
public static Collection<Object[]> data()
{
String tableSchema = withKeyspace("CREATE TABLE %s.tbl (pk int PRIMARY KEY, v1 text)");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this actually working?

Yes. If it isn't cassci-bot will report a failure as distributed tests are run in CI. Needless to say it works locally.

All tables are created/destroyed per test permutation, it just happens this one has a different schema.

The reason tables are created in each test permutation is to isolate sensors (won't make much sense for example to read -> assert READ_BYTES -> range read --> assert READ_BYTES unless we opt in for testing exact increments of sensor values which is not really the intent of this test).

assertThat(onResponseStartSignal[replica].await(1, TimeUnit.SECONDS)).isTrue();
}

private void testPrepareCallbackSensorsTracking(Message writeRequest, boolean allowHints) throws InterruptedException
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was trying to communicate the fact that the test dimensions are generally the callback type used (rather than the verb or the Message type, with few exceptions :/) because this is how sensors are tracked in ResponseVerbHandler) but a better name is warranted.

assertThat(onResponseStartSignal[replica].await(1, TimeUnit.SECONDS)).isTrue();
}

private void testPrepareCallbackSensorsTracking(Message writeRequest, boolean allowHints) throws InterruptedException
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cassci-bot
Copy link

✔️ Build ds-cassandra-pr-gate/PR-1295 approved by Butler


Approved by Butler
See build details here

@jkni jkni self-requested a review December 18, 2024 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants