Re: [PR] fix: prevent concurrent access to non-thread-safe maps in SketchBufferAggregatorHelper (druid)

["zhang-arvin (via GitHub)" <gi...@apache.org>]

zhang-arvin commented on PR #20090:
URL: https://github.com/apache/druid/pull/20090#issuecomment-5778664739

   Thanks @FrankChen021 — this is a real correctness bug and the fix is in 8576e126.
   
   You are right that `ConcurrentHashMap` is the wrong structure here. It keys by `ByteBuffer.equals`/`hashCode`, which are derived from the buffer's *remaining contents*. `Union` updates mutate exactly those contents in place through the `WritableMemory` wrapping the same buffer, so an inserted key's hash changes as the aggregation progresses. The consequences are the ones you described: `unions.get(buf)` misses after the first mutation, `get()` returns `SketchHolder.EMPTY`, and two distinct buffers holding equal contents alias onto each other.
   
   I went back to identity keying rather than trying to stabilise a content-derived key, because identity is the semantics this cache actually wants — the key is a specific arena instance, not a value:
   
   ```java
   private final IdentityHashMap<ByteBuffer, Int2ObjectMap<Union>> unions = new IdentityHashMap<>();
   private final IdentityHashMap<ByteBuffer, WritableMemory> memCache = new IdentityHashMap<>();
   private final Object lock = new Object();
   ```
   
   `IdentityHashMap` is not thread-safe, so every access is now guarded by a monitor. That addresses the non-thread-safe access the previous change was *intended* to fix, without reintroducing the content-derived key. The critical sections are plain map operations on the query path, so a single monitor should not become a contention point — though if you'd prefer an RWMutex or per-buffer striping I'm happy to change it.
   
   Also note `createNewUnionLocked` restores the `get`-then-`put` shape instead of `computeIfAbsent`: with `IdentityHashMap` there is no atomic primitive, hence the explicit lock and the `Locked` suffix on the helper to make the caller's obligation visible. `relocate`, `getOrCreateUnion`, `createNewUnion` and `clear` all hold the lock for their whole body so the multi-step invariants stay atomic.
   
   **Verification:** `mvn -pl extensions-core/datasketches -am package -DskipTests` → BUILD SUCCESS (whole dependency chain, including the changed module).


-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org