wang-jiahua commented on code in PR #10528:
URL: https://github.com/apache/rocketmq/pull/10528#discussion_r3878053987
##########
store/src/main/java/org/apache/rocketmq/store/index/IndexService.java:
##########
@@ -215,10 +216,12 @@ public QueryOffsetResult queryOffset(String topic, String
key, int maxNum, long
}
private String buildKey(final String topic, final String key) {
- return topic + "#" + key;
+ reusableKeyBuilder.setLength(0);
Review Comment:
Confirmed — `buildKey` is reachable from `queryOffset` (query thread pools)
and from concurrent dispatch when `enableBuildConsumeQueueConcurrently` is on,
so a shared instance StringBuilder can interleave. Switched to
`ThreadLocal<StringBuilder>` in 43be17b98, which keeps the allocation win on
the hot build path and is safe on every thread.
##########
store/src/main/java/org/apache/rocketmq/store/timer/TimerWheel.java:
##########
@@ -128,17 +130,25 @@ public void flush() {
if (mappedByteBuffer == null) {
return;
}
+ if (!dirty) {
Review Comment:
The per-thread-buffer scenario doesn't apply here: `localBuffer` holds
`byteBuffer.duplicate()`, and duplicates share the single underlying direct
buffer — the ThreadLocal only isolates position/limit cursors, so there is
exactly one copy of the wheel data and no thread can flush stale/uninitialized
content. That said, this comment made me re-examine the flag ordering and there
was a real lost-update window: clearing `dirty` after the diff could swallow a
concurrent `putSlot`'s mark, leaving the last write unflushed until the next
putSlot. Fixed in 43be17b98 by clearing the flag before diffing — any
interleaving now causes at most one extra flush, never a missed one.
##########
store/src/main/java/org/apache/rocketmq/store/queue/QueueOffsetOperator.java:
##########
@@ -123,10 +132,18 @@ public void setLmqTopicQueueTable(ConcurrentMap<String,
Long> lmqTopicQueueTable
}
public ConcurrentMap<String, Long> getTopicQueueTable() {
- return topicQueueTable;
+ ConcurrentMap<String, Long> snapshot = new
ConcurrentHashMap<>(this.topicQueueTable.size() * 2);
Review Comment:
Agreed, and on reflection the risk is worse than a doc issue: callers
(including extensions that reach this via reflection, e.g. recovery flows
pre-populating offsets) may rely on mutating the returned map and would break
silently with a snapshot. I reverted the QueueOffsetOperator changes entirely
in 43be17b98 — `getTopicQueueTable()` returns the live map again and the
AtomicLong conversion is dropped from this PR.
--
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]
