absurdfarce commented on code in PR #2037:
URL:
https://github.com/apache/cassandra-java-driver/pull/2037#discussion_r2103068297
##########
core/src/main/java/com/datastax/oss/driver/api/core/context/DriverContext.java:
##########
@@ -139,6 +140,10 @@ default SpeculativeExecutionPolicy
getSpeculativeExecutionPolicy(@NonNull String
@NonNull
RequestTracker getRequestTracker();
+ /** @return The driver's request ID generator; never {@code null}. */
+ @NonNull
+ RequestIdGenerator getRequestIdGenerator();
Review Comment:
I'm going to argue this should actually return
`Optional<RequestIdGenerator>`. I think part of the confusion for various
other aspects of this ticket come down to (a) an impl which requires the driver
to _always_ have a request ID generator and (b) a confusion between a log
prefix in the driver and what we're sending as a request ID.
##########
core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandler.java:
##########
@@ -139,7 +144,9 @@ protected CqlRequestHandler(
String sessionLogPrefix) {
this.startTimeNanos = System.nanoTime();
- this.logPrefix = sessionLogPrefix + "|" + this.hashCode();
+ this.requestIdGenerator = context.getRequestIdGenerator();
+ this.logPrefix =
+ this.requestIdGenerator.getSessionRequestId(statement,
sessionLogPrefix, this.hashCode());
Review Comment:
I think this is the root cause of my problem with the API. I think we need
to clearly distinguish between a log prefix and a request ID. If a user
doesn't configure a request ID generator that's totally fine... that means:
* Nothing is added to custom payload AND
* The old logic for generating a logPrefix is employed
That means our request ID generator API doesn't have to be retrofitted to
support the existing log prefix syntax. It also resolve the issue @aratno has
raised elsewhere, specifically "how do we shut this off if we don't want it?"
##########
core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandler.java:
##########
@@ -248,6 +259,19 @@ private void sendRequest(
if (result.isDone()) {
return;
}
+ String nodeRequestId =
+ this.requestIdGenerator.getNodeRequestId(statement, logPrefix,
currentExecutionIndex);
+ if (!this.customPayloadKey.isEmpty()) {
+ Map<String, ByteBuffer> customPayload =
+ NullAllowingImmutableMap.<String, ByteBuffer>builder()
+ .putAll(statement.getCustomPayload())
+ .put(
+ this.customPayloadKey,
+
ByteBuffer.wrap(nodeRequestId.getBytes(StandardCharsets.UTF_8)))
+ .build();
+ // TODO: we are creating a new statement object for every request. We
should optimize this.
+ statement = statement.setCustomPayload(customPayload);
+ }
Review Comment:
This is the wrong place to do this. In most cases we haven't even selected
the node yet; note that this happens immediately below where we poll the query
plan if no node is explicitly set in the request. Assuming we update the
request ID generation logic to correctly account for the target node the
setting of custom payload fields should happen _after_ we determine which node
we're actually sending to.
##########
core/src/main/resources/reference.conf:
##########
@@ -918,6 +918,15 @@ datastax-java-driver {
}
}
+ advanced.request-id{
+ generator{
+ # The component that generates a unique identifier for each CQL request.
+ class = DefaultRequestIdGenerator
+ }
+ # add the request id to the custom payload with the given key
+ # if empty, the request id will not be added to the custom payload
+ custom-payload-with-key = ""
Review Comment:
Presumably this will vary with the implementation, right @aratno?
Individaul C* request handlers might want to map this value to some name that
makes sense for them. So I guess this would be very
implementation-dependent... ?
Side note: it does raise an interesting question for Astra actually. We'd
want to automatically set a request ID generator if the user is using Astra...
but that's only half the problem. In addition to generating IDs in the
expected format we'd also want to make sure the custom payload is being added
_at the right key_ for Astra. Hmmm... that's an interesting problem.
##########
core/src/main/java/com/datastax/oss/driver/api/core/tracker/RequestIdGenerator.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.datastax.oss.driver.api.core.tracker;
+
+import com.datastax.oss.driver.api.core.session.Request;
+import edu.umd.cs.findbugs.annotations.NonNull;
+
+public interface RequestIdGenerator {
+ /**
+ * Generates a unique identifier for the session request. This will be the
identifier for the
+ * entire `session.execute()` call. This identifier will be added to logs,
and propagated to
+ * request trackers.
+ *
+ * @param statement the statement to be executed
+ * @param sessionName the name of the session
+ * @param hashCode the hashcode of the CqlRequestHandler
+ * @return a unique identifier for the session request
+ */
+ String getSessionRequestId(@NonNull Request statement, @NonNull String
sessionName, int hashCode);
+
+ /**
+ * Generates a unique identifier for the node request. This will be the
identifier for the CQL
+ * request against a particular node. There can be one or more node requests
for a single session
+ * request, due to retries or speculative executions. This identifier will
be added to logs, and
+ * propagated to request trackers.
+ *
+ * @param statement the statement to be executed
+ * @param sessionRequestId the session request identifier
+ * @param executionCount the number of previous node requests for this
session request, due to
+ * retries or speculative executions
+ * @return a unique identifier for the node request
+ */
+ String getNodeRequestId(
+ @NonNull Request statement, @NonNull String sessionRequestId, int
executionCount);
Review Comment:
In related news: how do we not include the node in question when we're
generating a node request ID? Requests/Statements can have a node set as state
but that's an optional thing a user can set in order to target a specific node;
that's not automatically set for every request.
##########
core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java:
##########
@@ -994,7 +994,21 @@ public enum DefaultDriverOption implements DriverOption {
*
* <p>Value-type: boolean
*/
-
SSL_ALLOW_DNS_REVERSE_LOOKUP_SAN("advanced.ssl-engine-factory.allow-dns-reverse-lookup-san");
+
SSL_ALLOW_DNS_REVERSE_LOOKUP_SAN("advanced.ssl-engine-factory.allow-dns-reverse-lookup-san"),
+
+ /**
+ * The class of session-wide component that generates request IDs.
+ *
+ * <p>Value-type: {@link String}
+ */
+ REQUEST_ID_GENERATOR_CLASS("advanced.request-id.generator.class"),
+
+ /**
+ * If not empty, the driver will write the node request ID to this key in
the custom payload
+ *
+ * <p>Value-type: {@link String}
+ */
+ REQUEST_ID_CUSTOM_PAYLOAD_KEY("advanced.request-id.custom-payload-with-key");
Review Comment:
I believe @aratno is referring to the TypeSafe name @SiyaoIsHiding ...
"custom-payload-key" rather than "custom-payload-with-key". Assuming that's
correct I think he's on to something there.
##########
core/src/main/java/com/datastax/oss/driver/api/core/tracker/RequestIdGenerator.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.datastax.oss.driver.api.core.tracker;
+
+import com.datastax.oss.driver.api.core.session.Request;
+import edu.umd.cs.findbugs.annotations.NonNull;
+
+public interface RequestIdGenerator {
+ /**
+ * Generates a unique identifier for the session request. This will be the
identifier for the
+ * entire `session.execute()` call. This identifier will be added to logs,
and propagated to
+ * request trackers.
+ *
+ * @param statement the statement to be executed
+ * @param sessionName the name of the session
+ * @param hashCode the hashcode of the CqlRequestHandler
+ * @return a unique identifier for the session request
+ */
+ String getSessionRequestId(@NonNull Request statement, @NonNull String
sessionName, int hashCode);
+
+ /**
+ * Generates a unique identifier for the node request. This will be the
identifier for the CQL
+ * request against a particular node. There can be one or more node requests
for a single session
+ * request, due to retries or speculative executions. This identifier will
be added to logs, and
+ * propagated to request trackers.
+ *
+ * @param statement the statement to be executed
+ * @param sessionRequestId the session request identifier
+ * @param executionCount the number of previous node requests for this
session request, due to
+ * retries or speculative executions
+ * @return a unique identifier for the node request
+ */
+ String getNodeRequestId(
+ @NonNull Request statement, @NonNull String sessionRequestId, int
executionCount);
Review Comment:
Sure, but that doesn't mean execution count is relevant to all
implementations. It also begs the question of whether other things can/should
be included for all implementations.
More generally, I'd argue it's inclusion here is primarily a function of the
necessity of implementing the current log prefix as a request ID generator...
which I'm not sure is a good idea (more on that elsewhere).
##########
core/src/main/resources/reference.conf:
##########
@@ -918,6 +918,15 @@ datastax-java-driver {
}
}
+ advanced.request-id{
+ generator{
+ # The component that generates a unique identifier for each CQL request.
+ class = DefaultRequestIdGenerator
+ }
+ # add the request id to the custom payload with the given key
+ # if empty, the request id will not be added to the custom payload
+ custom-payload-with-key = ""
Review Comment:
I'd actually collapse your second and third cases into one @aratno. I'd
also specify the rule a bit differently:
_If the client has configured a request ID generator we'll use that to
generate a consistent request ID via the log prefix on the client side and the
custom payload params delivered to the server. Otherwise we'll preserve the
current log prefix on the client side and add nothing to the custom payload._
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
