[#10421] Fix Timer message rocksdb use wrong cache key.

[echooymxq]

Fix #10421

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.01%. Comparing base (2a9560c) to head (e6bf55a).
⚠️ Report is 8 commits behind head on develop.

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10422      +/-   ##
=============================================
- Coverage      48.05%   48.01%   -0.04%     
+ Complexity     13303    13299       -4     
=============================================
  Files           1377     1377              
  Lines         100611   100612       +1     
  Branches       12991    12991              
=============================================
- Hits           48347    48310      -37     
  Misses         46348    46348              
- Partials        5916     5954      +38     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[echooymxq]

@zk-drizzle help review it.

[oss-sentinel-ai]

Review by github-manager-bot

Summary

Fixes #10421 — DELETE_KEY_CACHE_FOR_TIMER used raw byte[] as Guava cache key, causing identity-based lookups to always miss. Switches to ByteBuffer for content-equality semantics.

Findings

• [Correctness] ✅ Changing cache key from byte[] to ByteBuffer properly uses content-based equals()/hashCode(), which is the correct fix for the ghost record issue.
• [Correctness] ⚠️ Line 87-88: The cache declaration Cache<byte[], byte[]> appears unchanged in the diff — please verify the cache type is also updated to Cache<ByteBuffer, byte[]> to match the ByteBuffer.wrap() calls at lines 357 and 359.
• [Correctness] ⚠️ ByteBuffer.wrap(keyBytes).equals() compares from position() to limit(). Since wrap() sets position=0 and limit=array.length, this works correctly for equal-length arrays. However, if key lengths could differ, consider using ByteBuffer.wrap(keyBytes, offset, length) consistently.
• [Tests] ⚠️ No new tests visible in this diff. The bug is about cache key identity vs equality — recommend adding a unit test that verifies DELETE + UPDATE for the same timer key results in the UPDATE being correctly skipped.
• [Compatibility] ✅ No public API changes. Internal implementation detail only.

Suggestions

1. Ensure cache type declaration matches usage: Cache<ByteBuffer, byte[]> (not Cache<byte[], byte[]>)
2. Add a test case that creates a DELETE record and an UPDATE record for the same timer key, and verifies the UPDATE does not re-insert the deleted key
3. Consider adding a brief comment explaining why ByteBuffer is used instead of byte[] (identity vs content equality)

---

Automated review by github-manager-bot

[3424672656]

lgtm

[RockteMQ-AI]

Review by github-manager-bot

Summary

Fixes a cache key bug in Timer message RocksDB storage where byte[] keys were used in a Guava Cache, causing cache misses due to Java's reference-based byte[] equality.

Findings

• [Critical] MessageRocksDBStorage.java:87 — Root cause: Cache<byte[], byte[]> uses reference equality for byte[] keys. DELETE_KEY_CACHE_FOR_TIMER.getIfPresent(keyBytes) would never match a previously stored entry because each byte[] is a distinct object. Changing to Cache<String, byte[]> with a stable delayTime:uniqKey string key is the correct fix.
• [Info] MessageRocksDBStorage.java:376 — getTimerCacheKey() allocates a new String on every call. For the timer write hot path, consider whether the allocation is acceptable. Given the correctness fix is more important, this is fine for now.
• [Info] MessageRocksDBStorageTest.java — Good test coverage: 3 scenarios (PUT→DELETE, PUT→UPDATE, DELETE→UPDATE) validate the fix. Tests correctly verify record counts after each operation.

Suggestions

• Consider adding a test case that verifies the cache actually prevents a redundant write (i.e., the UPDATE after DELETE is skipped because the cache hit returns DELETE_VAL_FLAG). The existing testDeleteThenUpdate already covers this implicitly.

Verdict: LGTM. Clean bug fix with correct root cause analysis and good test coverage.

---

Automated review by github-manager-bot