leizhiyuan opened a new pull request, #1437:
URL: https://github.com/apache/rocketmq-clients/pull/1437
### Which Issue(s) This PR Fixes
Fixes #1436
> [!IMPORTANT]
> **Behavior change, please confirm before merging (see #1436).**
`MessageView::born_timestamp()` and `delivery_timestamp()` change from seconds
to milliseconds, and `delivery_timestamp()` becomes `None` for normal messages.
Code that uses the old values keeps compiling but gets different numbers.
### Brief Description
**Scenario.** An application computes the message age (`now -
born_timestamp()`), or decides from `delivery_timestamp()` whether a message is
a delay message. With the Rust client today, the age is off by a factor of 1000
and every message looks like a delay message. For a delay message, the delivery
time is the proxy's batch delivery time, not the time the message was scheduled
for. Details in #1436.
**Root causes.**
1. `MessageView::from_pb_message` keeps only `Timestamp.seconds` of
`born_timestamp` and `delivery_timestamp`.
2. `Client::receive_message_inner` copies the response-level
`delivery_timestamp` (the transport time of the batch) into the
`delivery_timestamp` system property of every message.
3. The producer sends `born_timestamp` with `nanos: 0`.
**Change.** Align with Java:
- `born_timestamp()` / `delivery_timestamp()` return milliseconds since the
Unix epoch, converted like protobuf-java `Timestamps.toMillis`
(`MessageViewImpl#fromProtobuf`). The doc comments state the unit.
- `receive_message_inner` no longer writes the response-level delivery
timestamp into the messages. `delivery_timestamp()` is now `None` for a normal
message and the configured time for a delay message. Java keeps the response
value as `transportDeliveryTimestamp`, which is only used by its
consume-latency metric. The Rust client has no such metric, so the value is
dropped, the same as the push consumer already does.
- The producer sends `born_timestamp` with millisecond precision, like Java
`PublishingMessageImpl` (`Timestamps.fromMillis`).
- `MessageBuilder::set_delivery_timestamp` / `delay_message_builder` are
unchanged and still take **seconds**. Their doc comments now say so.
#### Notes for reviewers
- **Please confirm**:
1. The unit change of the two public getters.
2. Dropping the transport delivery timestamp instead of exposing it
separately.
3. Whether the producer-side `set_delivery_timestamp` should also move to
milliseconds, as in Java.
I left the setter alone on purpose. If it changed silently, an existing
seconds value would be read as a time in January 1970 and the message delivered
immediately, so it needs a decision (maybe a new method). After this PR, the
getter is in ms and the setter is in s.
- **Scope.** The push consumer worker already ignores the response-level
delivery timestamp, and it shares `MessageView::from_pb_message`, so it gets
the millisecond getters too. The transaction-check `MessageView` (producer)
goes through the same function.
- **Conflicts.** `git merge-tree` is clean against all open Rust PRs: #1381,
#1404, #1407, #1409, #1411, #1414, #1416, #1420, #1421, #1423, #1425, #1427,
#1429, #1431, #1433, #1435. I also ran `cargo test --lib` on temporary merges
with the PRs that touch `client.rs`, `model/message.rs` or `producer.rs`: #1381
99 passed, #1404 100, #1416 96, #1420 92, #1423 110, #1429 99, #1431 93, #1433
94. On the #1381 and #1404 merges, nothing copies the response delivery
timestamp into messages any more.
### How Did You Test This Change?
**Unit tests.** All three fail on `master`:
- `model::message::tests::common_message` (updated): `born_timestamp {
seconds: 1_700_000_000, nanos: 123_456_789 }` gives `1_700_000_000_123`;
`delivery_timestamp` gives milliseconds.
- `client::tests::client_receive_message_keeps_message_delivery_timestamp`:
a response with a transport delivery timestamp, one delay message and one
normal message. The delay message keeps its own timestamp, and the normal
message stays `None`.
- `producer::tests::producer_born_timestamp_has_millisecond_precision`: the
sent `born_timestamp` is within the milliseconds measured around the call.
```
$ cargo test -- --test-threads=1
test result: ok. 91 passed; 0 failed; 1 ignored (lib; master: 89)
test result: ok. 3 passed; 0 failed (lite_integration_test)
test result: ok. 2 passed; 0 failed; 3 ignored (doc-tests)
$ cargo fmt --check # clean
$ cargo clippy --all-targets # same warnings as
master
$ cargo clippy --all-features -- -D warnings # only the 2
existing for_kv_map errors of master
$ RUSTDOCFLAGS='-D warnings' cargo doc --no-deps # no warnings
$ cargo build --examples --all-features # ok
```
**End-to-end.** Against a managed RocketMQ 5 server, with a normal topic and
group (SimpleConsumer). The same program ran on `master` and on this branch,
with the Rust producer and consumer:
| case | `master` | this PR | Java 5.2.2 |
|---|---|---|---|
| normal message `born_timestamp()` | `1792575801` (s) | `1792576084774`,
inside the send window `[1792576084774, 1792576085080]` | ms |
| normal message `delivery_timestamp()` | `Some(1792575801)` (transport
time) | `None` | `Optional.empty` |
| delay message `delivery_timestamp()` (set to `now + 12 s`) |
`Some(1792575841)`: transport time, 8 s after the configured `1792575833` |
`Some(1792576117000)`, equal to the configured time | configured time, ms |
| delay message `born_timestamp()` | `1792575821` (s) | `1792576105195` (ms)
| ms |
--
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]
