aratno commented on code in PR #2037:
URL:
https://github.com/apache/cassandra-java-driver/pull/2037#discussion_r2098849459
##########
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:
Style nit - elsewhere we have a space before the opening brace `{`
##########
manual/core/request_id/README.md:
##########
@@ -0,0 +1,69 @@
+<!--
+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.
+-->
+
+## Request Id
+
+### Quick overview
+
+Users can inject an identifier for each individual CQL request, and such ID
can be written in to the custom payload to
+correlate a request across the driver and the Apache Cassandra server.
+
+A request ID generator needs to generate both:
+- Session request ID: an identifier for an entire session.execute() call
+- Node request ID: an identifier for the execution of a CQL statement against
a particular node. There can be one or more node requests for a single session
request, due to retries or speculative executions.
Review Comment:
Retries and speculative executions are often against different nodes than
the original request, might prefer another name here like "Request Attempt ID".
Currently the server has no way to know whether a given request is a retry,
this feature could help us provide a metric for original requests vs. retries
on the server, which would be pretty cool.
##########
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:
Is there a way to disable Request IDs altogether? Seems like at least three
possible states are needed:
1. Disabled Request IDs, no behavior changes on upgrade
2. Request IDs in driver logs only, not propagated to the server
3. Request IDs in driver logs and propagated to the server
##########
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:
(nit) naming: I find "custom-payload-key" clearer, you're already using that
naming elsewhere
##########
manual/core/request_id/README.md:
##########
@@ -0,0 +1,69 @@
+<!--
+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.
+-->
+
+## Request Id
+
+### Quick overview
+
+Users can inject an identifier for each individual CQL request, and such ID
can be written in to the custom payload to
+correlate a request across the driver and the Apache Cassandra server.
Review Comment:
Could link to the spec:
https://github.com/apache/cassandra/blob/trunk/doc/native_protocol_v5.spec
##########
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);
Review Comment:
Could move this within the conditional below so it's not generated if not
enabled
##########
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:
How would a user know what to set this to? Can we come up with a reasonable
default that's more likely to be interoperable between C* protocol
implementations (C*, Scylla, DSE / Astra, etc)?
--
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-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org
For additional commands, e-mail: pr-h...@cassandra.apache.org
