lukasz-antoniak commented on code in PR #2037:
URL:
https://github.com/apache/cassandra-java-driver/pull/2037#discussion_r2046094444
##########
core/revapi.json:
##########
@@ -7386,6 +7386,11 @@
"old": "method <T extends java.lang.Number>
com.datastax.oss.driver.api.core.type.reflect.GenericType<com.datastax.oss.driver.api.core.data.CqlVector<T>>
com.datastax.oss.driver.api.core.type.reflect.GenericType<T>::vectorOf(java.lang.Class<T>)",
"new": "method <T>
com.datastax.oss.driver.api.core.type.reflect.GenericType<com.datastax.oss.driver.api.core.data.CqlVector<T>>
com.datastax.oss.driver.api.core.type.reflect.GenericType<T>::vectorOf(java.lang.Class<T>)",
"justification": "JAVA-3143: Extend driver vector support to arbitrary
subtypes and fix handling of variable length types (OSS C* 5.0)"
+ },
+ {
+ "code": "java.method.addedToInterface",
+ "new": "method
com.datastax.oss.driver.api.core.tracker.DistributedTraceIdGenerator
com.datastax.oss.driver.api.core.context.DriverContext::getDistributedTraceIdGenerator()",
+ "justification": "DistributedRequestID"
Review Comment:
Maybe reference JIRA ticket here?
##########
core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandler.java:
##########
@@ -248,6 +259,16 @@ private void sendRequest(
if (result.isDone()) {
return;
}
+ String nodeRequestId =
this.distributedTraceIdGenerator.getNodeRequestId(statement, logPrefix);
+ if (!this.customPayloadKey.isEmpty()) {
+ // We cannot do statement.getCustomPayload().put() because the default
empty map is abstract
+ // But this will create new Statement instance for every request. We
might want to optimize
+ // this
+ Map<String, ByteBuffer> existingMap = new
HashMap<>(statement.getCustomPayload());
Review Comment:
`Statement` is by design immutable. Maybe a nicer way would be to create
method `StatementBuilder.from(Statement)` where you could create builder again
based on statement. The code would look like:
`StatementBuilder.from(statement).addCustomPayload(...).build()`.
##########
core/src/main/java/com/datastax/oss/driver/internal/core/tracker/W3CContextDistributedTraceIdGenerator.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.internal.core.tracker;
+
+import com.datastax.oss.driver.api.core.context.DriverContext;
+import com.datastax.oss.driver.api.core.session.Request;
+import com.datastax.oss.driver.api.core.tracker.DistributedTraceIdGenerator;
+import com.datastax.oss.driver.shaded.guava.common.io.BaseEncoding;
+import edu.umd.cs.findbugs.annotations.NonNull;
+import java.util.Random;
+
+public class W3CContextDistributedTraceIdGenerator implements
DistributedTraceIdGenerator {
+ Random random = new Random();
Review Comment:
Rather use `SecureRandom`.
##########
core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandler.java:
##########
@@ -248,6 +259,16 @@ private void sendRequest(
if (result.isDone()) {
return;
}
+ String nodeRequestId =
this.distributedTraceIdGenerator.getNodeRequestId(statement, logPrefix);
+ if (!this.customPayloadKey.isEmpty()) {
+ // We cannot do statement.getCustomPayload().put() because the default
empty map is abstract
+ // But this will create new Statement instance for every request. We
might want to optimize
+ // this
+ Map<String, ByteBuffer> existingMap = new
HashMap<>(statement.getCustomPayload());
+ existingMap.put(
+ this.customPayloadKey,
ByteBuffer.wrap(nodeRequestId.getBytes(StandardCharsets.UTF_8)));
+ statement = statement.setCustomPayload(existingMap);
Review Comment:
Overriding custom payload here is not thread-safe. If client application
executes the same statement instance multiple times concurrently (not a good
use-case, but still possible), we do not guarantee how this map will be
changed. Maybe indeed, there is no other way than make a shallow copy of the
statement. Will think about it.
```
/**
* Sets the custom payload to use for execution.
*
* <p>All the driver's built-in statement implementations are immutable,
and return a new instance
* from this method. However custom implementations may choose to be
mutable and return the same
* instance.
*
* <p>Note that it's your responsibility to provide a thread-safe map.
This can be achieved with a
* concurrent or immutable implementation, or by making it effectively
immutable (meaning that
* it's never modified after being set on the statement).
*/
@NonNull
@CheckReturnValue
SelfT setCustomPayload(@NonNull Map<String, ByteBuffer> newCustomPayload);
```
##########
core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandler.java:
##########
@@ -139,7 +144,8 @@ protected CqlRequestHandler(
String sessionLogPrefix) {
this.startTimeNanos = System.nanoTime();
- this.logPrefix = sessionLogPrefix + "|" + this.hashCode();
+ this.distributedTraceIdGenerator =
context.getDistributedTraceIdGenerator();
+ this.logPrefix =
this.distributedTraceIdGenerator.getSessionRequestId(statement);
Review Comment:
Can we maintain the `logPrefix` logic in default
`NoopDistributedTraceIdGenerator`? This prefix is inserted into logs, so it may
be nice to preserve backward compatibility.
--
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
