You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "wang-jiahua (via GitHub)" <gi...@apache.org> on 2026/08/27 08:45:45 UTC
[PR] [ISSUE #10972] Encode timer message propertiesString after internal properties are cleared [rocketmq]
wang-jiahua opened a new pull request, #10974:
URL: https://github.com/apache/rocketmq/pull/10974
### Which Issue(s) This PR Fixes
Fixes #10972
### Brief Description
`TimerMessageStore#convertMessage` encoded `propertiesString` from the original property map and only afterwards cleared `REAL_TOPIC` / `REAL_QUEUE_ID` from the deep copy. As a result the delivered timer message carried the internal properties on the wire while the broker-side map had them cleared, and the file-based timer store observably diverged from `TimerMessageRocksDBStore#convertMessage`, which already clears first and encodes from the copied map.
This PR moves the encode after the clear block and encodes from `msgInner.getProperties()`, mirroring the RocksDB implementation. It also removes the map/wire mismatch where the deep copy was made but the encode still read the original map. `TIMER_DELIVER_MS` and other timer metadata are not in the clearing list, so the deliveryTimestamp exposed to gRPC consumers is unchanged.
### How Did You Test This Change?
- New regression test `testConvertMessagePropertiesStringMatchesProperties`: the delivered message drops `REAL_TOPIC`/`REAL_QUEUE_ID` from the wire and `propertiesString` equals the re-encoded map; the rolled message keeps them. `TimerMessageStoreTest` passes 11/11.
- 4-node cluster A/B (file-based timer store, `benchmark.timer.TimerProducer` 64 threads x 40 slots x 250 msgs/slot, 1 KiB, per arm clean store + page cache drop, 3 interleaved trials; the 2-minute first-slot offset isolates GC sampling to the pure delivery phase): every arm delivered 639,990/640,000 (report-sampling rounding) with zero send failures; delivery-phase young GC base 2/2/2 vs patch 1/2/2 — correctness intact, no regression.
--
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@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #10972] Encode timer message propertiesString after internal properties are cleared [rocketmq]
Posted by "RongtongJin (via GitHub)" <gi...@apache.org>.
RongtongJin merged PR #10974:
URL: https://github.com/apache/rocketmq/pull/10974
--
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@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #10972] Encode timer message propertiesString after internal properties are cleared [rocketmq]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10974:
URL: https://github.com/apache/rocketmq/pull/10974#issuecomment-5436958468
## [Codecov](https://app.codecov.io/gh/apache/rocketmq/pull/10974?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 48.51%. Comparing base ([`e348efa`](https://app.codecov.io/gh/apache/rocketmq/commit/e348efa66b08eb645ee123706ea6492fa9a3ad35?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`15c5193`](https://app.codecov.io/gh/apache/rocketmq/commit/15c519387f37c3212b110646fac386faf2eb6ffa?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## develop #10974 +/- ##
=============================================
- Coverage 48.56% 48.51% -0.06%
+ Complexity 13672 13656 -16
=============================================
Files 1381 1381
Lines 101475 101475
Branches 13190 13190
=============================================
- Hits 49286 49231 -55
- Misses 46185 46237 +52
- Partials 6004 6007 +3
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/rocketmq/pull/10974?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #10972] Encode timer message propertiesString after internal properties are cleared [rocketmq]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #10974:
URL: https://github.com/apache/rocketmq/pull/10974#discussion_r3870176893
##########
store/src/test/java/org/apache/rocketmq/store/timer/TimerMessageStoreTest.java:
##########
@@ -179,6 +179,30 @@ private static PutMessageResult transformTimerMessage(TimerMessageStore timerMes
return null;
}
+ @Test
+ public void testConvertMessagePropertiesStringMatchesProperties() throws Exception {
+ final TimerMessageStore timerMessageStore = createTimerMessageStore(null, true);
+
+ MessageExtBrokerInner msgExt = buildMessage(3000, "TimerTest_testConvertMessage", false);
+ MessageAccessor.putProperty(msgExt, MessageConst.PROPERTY_REAL_TOPIC, msgExt.getTopic());
+ MessageAccessor.putProperty(msgExt, MessageConst.PROPERTY_REAL_QUEUE_ID, "0");
+ msgExt.setPropertiesString(MessageDecoder.messageProperties2String(msgExt.getProperties()));
+ msgExt.setTopic(TimerMessageStore.TIMER_TOPIC);
+
+ // delivered message: internal properties are cleared from both the map and the wire data
+ MessageExtBrokerInner delivered = timerMessageStore.convertMessage(msgExt, false);
+ assertEquals("TimerTest_testConvertMessage", delivered.getTopic());
+ assertFalse(delivered.getPropertiesString().contains(MessageConst.PROPERTY_REAL_TOPIC));
+ assertFalse(delivered.getPropertiesString().contains(MessageConst.PROPERTY_REAL_QUEUE_ID));
+ assertEquals(MessageDecoder.messageProperties2String(delivered.getProperties()), delivered.getPropertiesString());
+
+ // rolled message: keeps REAL_TOPIC and stays consistent between the map and the wire data
+ MessageExtBrokerInner rolled = timerMessageStore.convertMessage(msgExt, true);
+ assertEquals(TimerMessageStore.TIMER_TOPIC, rolled.getTopic());
+ assertTrue(rolled.getPropertiesString().contains(MessageConst.PROPERTY_REAL_TOPIC));
+ assertEquals(MessageDecoder.messageProperties2String(rolled.getProperties()), rolled.getPropertiesString());
Review Comment:
The test comment says the rolled message keeps both REAL_TOPIC and REAL_QUEUE_ID, but the assertions only verify REAL_TOPIC. Adding an assertion for REAL_QUEUE_ID would make this regression test fully cover the stated behavior.
--
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@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #10972] Encode timer message propertiesString after internal properties are cleared [rocketmq]
Posted by "wang-jiahua (via GitHub)" <gi...@apache.org>.
wang-jiahua commented on code in PR #10974:
URL: https://github.com/apache/rocketmq/pull/10974#discussion_r3877217177
##########
store/src/test/java/org/apache/rocketmq/store/timer/TimerMessageStoreTest.java:
##########
@@ -179,6 +179,30 @@ private static PutMessageResult transformTimerMessage(TimerMessageStore timerMes
return null;
}
+ @Test
+ public void testConvertMessagePropertiesStringMatchesProperties() throws Exception {
+ final TimerMessageStore timerMessageStore = createTimerMessageStore(null, true);
+
+ MessageExtBrokerInner msgExt = buildMessage(3000, "TimerTest_testConvertMessage", false);
+ MessageAccessor.putProperty(msgExt, MessageConst.PROPERTY_REAL_TOPIC, msgExt.getTopic());
+ MessageAccessor.putProperty(msgExt, MessageConst.PROPERTY_REAL_QUEUE_ID, "0");
+ msgExt.setPropertiesString(MessageDecoder.messageProperties2String(msgExt.getProperties()));
+ msgExt.setTopic(TimerMessageStore.TIMER_TOPIC);
+
+ // delivered message: internal properties are cleared from both the map and the wire data
+ MessageExtBrokerInner delivered = timerMessageStore.convertMessage(msgExt, false);
+ assertEquals("TimerTest_testConvertMessage", delivered.getTopic());
+ assertFalse(delivered.getPropertiesString().contains(MessageConst.PROPERTY_REAL_TOPIC));
+ assertFalse(delivered.getPropertiesString().contains(MessageConst.PROPERTY_REAL_QUEUE_ID));
+ assertEquals(MessageDecoder.messageProperties2String(delivered.getProperties()), delivered.getPropertiesString());
+
+ // rolled message: keeps REAL_TOPIC and stays consistent between the map and the wire data
+ MessageExtBrokerInner rolled = timerMessageStore.convertMessage(msgExt, true);
+ assertEquals(TimerMessageStore.TIMER_TOPIC, rolled.getTopic());
+ assertTrue(rolled.getPropertiesString().contains(MessageConst.PROPERTY_REAL_TOPIC));
+ assertEquals(MessageDecoder.messageProperties2String(rolled.getProperties()), rolled.getPropertiesString());
Review Comment:
Added the `PROPERTY_REAL_QUEUE_ID` assertion to the rolled-message branch so the test covers both retained properties (commit 20b4dc7).
--
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@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org