absurdfarce commented on code in PR #2060:
URL:
https://github.com/apache/cassandra-java-driver/pull/2060#discussion_r2506258957
##########
core/src/main/java/com/datastax/oss/driver/api/core/tracker/RequestIdGenerator.java:
##########
@@ -67,11 +68,15 @@ default String getCustomPayloadKey() {
default Statement<?> getDecoratedStatement(
@NonNull Statement<?> statement, @NonNull String requestId) {
- Map<String, ByteBuffer> customPayload =
- NullAllowingImmutableMap.<String, ByteBuffer>builder()
- .putAll(statement.getCustomPayload())
- .put(getCustomPayloadKey(),
ByteBuffer.wrap(requestId.getBytes(StandardCharsets.UTF_8)))
- .build();
- return statement.setCustomPayload(customPayload);
+
+ Map<String, ByteBuffer> existing = new
HashMap<>(statement.getCustomPayload());
+ String key = getCustomPayloadKey();
+
+ // Add or overwrite
+ existing.put(key,
ByteBuffer.wrap(requestId.getBytes(StandardCharsets.UTF_8)));
+
+ Map<String, ByteBuffer> unmodifiableMap =
Collections.unmodifiableMap(existing);
+
+ return statement.setCustomPayload(unmodifiableMap);
Review Comment:
I'm gonna make some notes here 'cause in all honesty this was pretty
perplexing to me... but I think I finally got to something resembling an
answer. I don't know how useful all of this will be but brain dump follows.
The
[doc](https://docs.datastax.com/en/developer/java-driver/3.6/manual/custom_payloads/index.html)
referenced by @tolbertam above is from the 3.6 Java driver, which was an
entirely different code base. The constant
[Statement.NULL_PAYLOAD_VALUE](https://github.com/apache/cassandra-java-driver/blob/3.6.0/driver-core/src/main/java/com/datastax/driver/core/Statement.java)
is indeed present in that version... so far so good. Thing is... there's no
corresponding entry in the 4.x manual and the [equivalent 4.x
class](https://apache.github.io/cassandra-java-driver/4.19.0/api/com/datastax/oss/driver/api/core/cql/Statement.html)
doesn't have such a constant. So... what's going on here?
Let's take a look at what the [v4 native protocol
spec](https://github.com/apache/cassandra/blob/trunk/doc/native_protocol_v4.spec)
actually says here. A `bytes map` is understood to have a `string` for a key
and `bytes` for a map. A `string` is understood to be `"A [short] n, followed
by n bytes representing an UTF-8 string"`. There's no obvious way for that to
be null, which is consistent with the verbiage in the 3.6 manual page. `bytes`
on the other hand is defined as follows: `"A [int] n, followed by n bytes if n
>= 0. If n < 0, no byte should follow and the value represented is null"`
That's presumably the point of the marker referenced in the 3.6 manual page.
To confirm all of this I ran a simple test client against C* 5.0.6 using
Java driver 4.19.1. In each case a custom payload was set on a simple
statement and executed. Results seem to follow the pattern described above:
when using a null key I get a weird native-protocol exception (an
[issue](https://github.com/datastax/native-protocol/issues/54) has been created
for that elsewhere) while a null value seems to work just fine.
So where does this leave us? In order to leverage ImmutableMap you need to
translate null values into zero-length ByteBuffers... but do so without
acquiring intermediate state. You can do it with something like the following:
```java
public class KeyspaceCount {
static class ImmutableEntry<K,V> implements Map.Entry<K,V> {
private final K key;
private final V val;
public ImmutableEntry(K key, V val) {
this.key = key;
this.val = val;
}
@Override
public K getKey() {
return key;
}
@Override
public V getValue() {
return val;
}
@Override
public V setValue(Object value) {
throw new UnsupportedOperationException("You can't do that");
}
}
public static void main(String[] args) {
try (CqlSession session = CqlSession.builder().build()) {
SimpleStatement stmt = SimpleStatement.builder("select
release_version from system.local").build();
HashMap<String, ByteBuffer> map = new HashMap<>();
map.put("key", ByteBuffer.allocate(0));
ImmutableMap.Builder<String,ByteBuffer> builder =
ImmutableMap.builder();
map.entrySet()
.stream()
.map((Map.Entry<String,ByteBuffer> entry) -> {
return entry.getValue() != null ?
entry :
new ImmutableEntry<>(entry.getKey(),
ByteBuffer.allocate(0));
})
.forEach(builder::put);
ResultSet rs =
session.execute(stmt.setCustomPayload(builder.buildKeepingLast()));
Row row = rs.one();
assert row != null;
String releaseVersion = row.getString("release_version");
System.out.printf("Cassandra version is: %s%n", releaseVersion);
}
}
}
```
That actually does work, and it's all stream-based so that the only map you
create is the one you're actually add to the Statement object. You could pull
the immutable Map.Entry impl into a utility class somewhere to help keep the
change down but in the end yeah, I agree it's not quite as clean as I'd like.
I'd still argue it's an improvement because (a) you avoid the intermediate maps
and (b) you get the immutable part reflected in the type system but these maps
are pretty short-lived anyway... so it probably doesn't matter that much.
--
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]
