[ISSUE #10651] Adapt RocketMQ to latest DLedger and remove fastjson1 - #10947
[ISSUE #10651] Adapt RocketMQ to latest DLedger and remove fastjson1#10947RongtongJin wants to merge 9 commits into
Conversation
Replace the local SNAPSHOT placeholder (0.3.3-pr336-f2-64-SNAPSHOT) with the officially released io.openmessaging.storage:dledger:0.4.3. This version includes all master-line APIs required by the adapter (batch append, fast commit-index, DLedgerEntryType, etc.) and transitively depends on fastjson2 2.0.64 with no fastjson1 leak in a full reactor build.
getCommittedPos() returns a negative value when no entry has been committed yet, which is exactly the state of a freshly started DLedger store layered on top of legacy commitlog data. The `committedPos == 0` guard therefore skipped the dLedgerFileList.getMinOffset() fallback that was previously applied for any non-positive committed position, so getMaxOffset() and getConfirmOffset() returned 0 instead of the divided commitlog offset. DefaultMessageStore then computed a negative dispatchBehindBytes() and left reputFromOffset behind, failing MixCommitlogTest#testPutAndGet with expected:<0> but was:<-1163232>. Also make StoreTestBase#nextPort() probe that a candidate port can actually be bound before handing it out. The counter previously returned port numbers blindly, so a stale listener in the 30000+ range made callers fail with "Address already in use"; the new three-node DLedger test needs three listeners at once and hit this on CI.
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
This PR adapts RocketMQ to the latest DLedger API (0.4.3) and removes the fastjson1 dependency in favor of fastjson2 2.0.64. The changes are well-structured and address a critical adaptation need.
Strengths:
- Clean API migration (package reorganization,
getCommittedIndex()moved toMemberState) - Proper committed boundary tracking with
getCommittedPos()caching prevents reading uncommitted data - Good NOOP/control entry handling during append, reput, and recovery
- Comprehensive test coverage including a 601-line three-broker failover IT
- fastjson1→fastjson2 migration with backward compatibility fixtures (
RemotingSerializableCompatTest)
Notes:
- The PR description mentions this is "intentionally a draft" pending DLedger PR #336 merge, but the PR is marked as ready for review on GitHub. Please confirm the intended status.
- The
calculate-coverageCI check is failing — this may need attention before merge. - The WORKSPACE file references
dledger:0.4.3while the PR body mentions0.3.3-pr336-f2-64-SNAPSHOT. Please clarify which version is the target.
Overall the code changes look solid and the adaptation is thorough. The test coverage is particularly impressive.
Automated review by "github-manager-bot"
Findings
-
[Info]
DLedgerCommitLog.java:75— ThecachedCommittedPos/cachedCommittedIndexcaching pattern is well-designed. Consider adding a brief Javadoc explaining the cache invalidation contract. -
[Info]
DLedgerCommitLog.java:247—firstNormalEntryBody()correctly skips NOOP entries during recovery. Good defensive handling of all-NOOP mapped files. -
[Warning]
DLedgerCommitLog.java:500— IngetMessage(), the guardcommittedPos < 0means no entry committed yet. Consider logging at DEBUG level when this triggers to help diagnose startup timing issues. -
[Info]
DefaultMessageStore.java:2741— The reput guarddispatchRequest.getMsgSize() > 0correctly prevents dispatching empty messages from NOOP/control entries.
dledger 0.3.3.4 depends on rocketmq-remoting:5.5.0, which Coursier fetches from Maven Central. That artifact transitively brings grpc-netty-shaded:1.53.0 with a strict [1.53.0] range on grpc-core, conflicting with the WORKSPACE-declared grpc 1.47.0 artifacts. Since rocketmq-remoting is built from source in this repo (//remoting), exclude it from external resolution — matching what PR #10947 already does for the DLedger 0.4.x line.
* chore: bump DLedger to 0.3.3.4 (maintenance line) Update the DLedger dependency from 0.3.2 to 0.3.3.4, the latest release on the 0.3.x maintenance line published on Maven Central. The 0.3.3.4 release is API-compatible with 0.3.2 and only carries dependency hygiene fixes (aligning fastjson2 and excluding fastjson1 from its rocketmq-remoting transitive path). Also update the Bazel WORKSPACE artifact coordinate to keep the two build systems in sync. * chore: fully remove fastjson1 from the build DLedger 0.3.3.4 already excludes com.alibaba:fastjson from its rocketmq-remoting dependency and its bytecode only references fastjson2, but rocketmq still declared fastjson1 itself, so it kept leaking onto every module compile classpath through rocketmq-common. - drop the com.alibaba:fastjson property and dependencyManagement entry from the root pom - drop the unused com.alibaba:fastjson dependency from rocketmq-common (no main source imports com.alibaba.fastjson.*) - drop the fastjson1 pin from WORKSPACE and the fastjson1 dep from the remoting test target in remoting/BUILD.bazel - migrate RemotingSerializableCompatTest to fastjson2: use the fastjson2 JSONField annotation (which is what the protocol classes actually carry) and round trip through RemotingSerializable instead of com.alibaba.fastjson.JSON fastjson1 wire-format coverage is retained by the frozen fastjson1 payload already asserted in testCompatibilityCheckWithBitSet. * fix(bazel): exclude rocketmq-remoting from coursier resolution dledger 0.3.3.4 depends on rocketmq-remoting:5.5.0, which Coursier fetches from Maven Central. That artifact transitively brings grpc-netty-shaded:1.53.0 with a strict [1.53.0] range on grpc-core, conflicting with the WORKSPACE-declared grpc 1.47.0 artifacts. Since rocketmq-remoting is built from source in this repo (//remoting), exclude it from external resolution — matching what PR #10947 already does for the DLedger 0.4.x line. --------- Co-authored-by: 通融 <rongtong.jrt@alibaba-inc.com>
|
This PR has conflicts with the git fetch origin
git checkout codex/dledger-latest-pr336-adapter
git rebase origin/develop
# resolve conflicts, then:
git push --force-with-leaseThis is a one-time reminder. Feel free to @mention me for a re-review after conflicts are resolved. Automated notification by "github-manager-bot" |
Which Issue(s) This PR Fixes
Brief Description
This PR reworks the DLedger 0.3.3 adaptation on top of the latest DLedger code and openmessaging/dledger#336. It is intended to supersede the incomplete adaptation in #10650.
The change:
AppendFutureresult for a single-message batch andBatchAppendFuturefor multi-message batches;The validated compatibility boundary is a coordinated full-stop upgrade. Mixed-version rolling upgrade and downgrade are not covered by this PR.
Important
This is intentionally a draft. The current DLedger coordinate,
0.3.3-pr336-f2-64-SNAPSHOT, is a locally built artifact from DLedger master + PR #336 + fastjson2 2.0.64. Before this PR can be merged, DLedger #336 must be merged and a publicly resolvable DLedger artifact must be published; this PR must then switch to that coordinate and rerun the matrix.Related work: #10650, #10928, openmessaging/dledger#336.
How Did You Test This Change?
All Java verification used Amazon Corretto JDK 8.
clean test: 19/19 modules, 3,124 tests, 0 failures, 0 errors, 42 skipped.git diff --checkpassed.