CASSJAVA-116: Retry or Speculative Execution with RequestIdGenerator throws "Duplicate Key"

[SiyaoIsHiding]

No description provided.

[tolbertam]

Just wondering, at this point what value is NullAllowingImmutableMap providing?

Since it is what has the duplicate key restriction, and it's main value is allowing null keys and values, but we would never set a null key/value in this context (correct me if I'm wrong?).

As far as I understand it, it's just ImmutableMap that has the null key/value restriction. Why wouldn't we just use a regular Map implementation and wrap it in Collections.unmodifiableMap?

[absurdfarce]

I did some spelunking and it looks like @emerkle826 made that mod way back in the day. I've already pinged him demanding that he explain himself asking very politely if we could provide the relevant context 🧌

[SiyaoIsHiding]

re: what map to use.

It seems like the protocol allows null values in the custom payload (source).

So the custom payload provided by the user can already have null values in it, and we need to support that.

But a normal ImmutableMap does not allow a null value. I'm guessing that's why we were using a NullAllowingImmutableMap. Pending for @olim7t and @emerkle826 to confirm.

However, Collections.unmodifiableMap is also immutable and allows null values. I cannot quite see why we don't use that.

The code here will be cleaner if we use Collections.unmodifiableMap. If no objections, I think we can go that way.

[emerkle826]

Just chiming in that when I added the NullAllowingImmutableMap, I believe I did it simply because it was the Map type we were using around changes made in Native Protocol related to #1087. I don't recall any comments or real reasons around choosing that Map implementation as opposed to some other immutable map that allowed nulls other than we were already using it in native protocol so why not. :)

[tolbertam]

👍 this looks like it should address the issue to me!

[lukasz-antoniak]

I think we should check here for absence of request-id (default), not trace_key (copied from upper test).

[lukasz-antoniak]

IMO this assertion is not needed in this test.

[lukasz-antoniak]

Left two NIT comments. PR looks good!

[tolbertam]

@SiyaoIsHiding looks like we got 2 +1s on the review 🎉

Mind implementing Lukasz's suggestions and then squashing into one commit and adding:

"patch by Jane He; reviewed by Andy Tolbert and Lukasz Atoniak for CASSJAVA-116" to the last line of the commit message?

[absurdfarce]

I don't know that I want to hold things up any but I'd argue ImmutableMap is more concise (and avoids the need to create at least one of the intermediate maps here):

  default Statement<?> getDecoratedStatement(
      @NonNull Statement<?> statement, @NonNull String requestId) {

      return statement.setCustomPayload(
              ImmutableMap.<String,ByteBuffer>builder()
              .putAll(statement.getCustomPayload())
              .put(getCustomPayloadKey(), ByteBuffer.wrap(requestId.getBytes(StandardCharsets.UTF_8)))
              .buildKeepingLast());
  }

[tolbertam]

The original map implementation that was used that caused this issue NullAllowingImmutableMap was presumably being used to allow null keys/values, which ImmutableMap doesn't allow, so if we use that it's possible any existing custom payload map with null keys and values will cause an exception to be thrown here (discussion here: #2060 (comment))

[absurdfarce]

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 referenced by @tolbertam above is from the 3.6 Java driver, which was an entirely different code base. The constant Statement.NULL_PAYLOAD_VALUE 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 doesn't have such a constant. So... what's going on here?

Let's take a look at what the v4 native protocol 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 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:

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.

[tolbertam]

Collections.unmodifiableWrap just wraps the underlying map with some API methods that prevent modification, so it's not like it's creating an expensive copy right? Or is there something I'm missing here?

[absurdfarce]

That's probably right @tolbertam but in the end I don't think it matters much. As I mentioned in my earlier comment my hope was that the Guava ImmutableMap code would be easier to read and comprehend but the necessity to map null values there makes the code more convoluted. You do get the advantage of having immutability as part of the type system but that doesn't but you much if you just stick the results in a Statement where custom payload is just a Map anyway.

Given all of that I'm okay with the original impl. My primary goal in this exercise was just to better understand (and document) how null handling even entered into play here. The driver docs you cited clearly indicated that null handling is possible (and relevant) here but I wanted to completely understand why that was the case.

[SiyaoIsHiding]

Agree with Andrew, Collections.unmodifiableMap only wraps and does not create a new map.
I added comments, hope it makes it easier to read.

[SiyaoIsHiding]

I addressed Lukasz' feedback on the test, and squashed the commits!

[absurdfarce]

@tolbertam and/or @lukasz-antoniak ... if you're good with @SiyaoIsHiding 's commit squash above I believe this guy is ready to merge