leizhiyuan opened a new pull request, #1425:
URL: https://github.com/apache/rocketmq-clients/pull/1425
### Which Issue(s) This PR Fixes
Fixes #1424
> [!IMPORTANT]
> **This PR depends on #1404. Please merge #1404 first.**
> The code it fixes (`PushConsumer::apply_settings` and
`SharedLongPollingTimeout`) only exists in #1404. This branch has one temporary
merge commit that brings in #1404 at `b5cb4940`. Its message also mentions
#1381, but this PR does not need #1381. The only commit to review is the last
one, `[ISSUE #1424] ...`, which touches `rust/src/push_consumer.rs` only. I
will rebase onto `master` and drop the merge commit once #1404 is in.
>
> To view only this change: `git diff HEAD~1 HEAD`, or the last commit in
the Commits tab.
### Brief Description
**Scenario.** The server pushes `long_polling_timeout = 20s`. The client
keeps the default of 40 s, with `timeout` 3 s. `PushConsumer` /
`LitePushConsumer` sends 20 s in the request, but some receive requests the
server does not answer at 20 s run until 43 s and end with `Timeout expired`.
This also slows down `shutdown()`, which waits for in-flight receive requests
(#1422 / #1423). Details in #1424.
**Root cause.** #1404 makes the session compute the receive deadline from
the client's shared `SharedLongPollingTimeout`, and `SimpleConsumer` writes the
server value into it. `PushConsumer::apply_settings`, also added in #1404, only
writes the server value into `PushConsumerOption`, which the request carries.
The shared value, and so the deadline, keeps the configured 40 s.
**Change.** This aligns with Java, where `ConsumerImpl#receiveMessage` uses
the same `awaitDuration` for the request and for the deadline (`awaitDuration +
requestTimeout`).
- `PushConsumer::start()` and `start_with_telemetry()` (used by
`LitePushConsumer`) take the client's `long_polling_timeout_handle()`.
- `apply_settings` writes the server `long_polling_timeout` into it, next to
`PushConsumerOption`.
The deadline is then server `long_polling_timeout + timeout`, 23 s here, as
in Java.
#### Notes for reviewers
- **Conflicts.** `git merge-tree` against the open Rust PRs:
- Clean with #1407, #1414, #1416, #1420, #1421 and #1423 (the shutdown
fix).
- It conflicts with #1381, #1409 and #1411, but only because of #1404,
which conflicts with those three already. This commit adds no conflict of its
own.
- With #1411, the conflict is new since #1404's latest push. Both add `let
_m = MTX.lock();` to `client_get_session`.
- Overview and merge order in #1412.
- An existing test, `test_push_consumer_start_shutdown`, now also mocks
`long_polling_timeout_handle()`.
### How Did You Test This Change?
**Unit tests.** Two tests are added. Each one starts a `PushConsumer` on a
mocked client, pushes a `Settings` command with `long_polling_timeout = 20s`
through the telemetry channel, and checks the client's shared long polling
timeout. One goes through `start()`, the other through
`start_with_telemetry()`. Without the fix, both fail with `left: 40s, right:
20s`, while `PushConsumerOption` already holds 20 s. Both passed 10 out of 10
repeated runs.
```
$ cargo test
test result: ok. 100 passed; 0 failed; 1 ignored (lib; #1404: 98)
... all other test targets pass
$ cargo fmt --check # clean
$ cargo clippy -- -D warnings # clean
$ RUSTDOCFLAGS="--cfg docsrs" cargo doc --no-deps --all-features # no
warnings
$ cargo build --examples # ok
```
`cargo clippy --all-targets` reports the same findings per file as #1404.
**End-to-end.** I ran this against a managed RocketMQ 5 server that pushes
`long_polling_timeout = 20s`. The client was built with #1381, #1407 (so the
consumer can receive on that server) and #1423 also merged. A
`LitePushConsumer` with 6 assigned queues sits idle for 35 s, then calls
`shutdown()`:
| | longest receive request | receive requests ended by `Timeout expired` |
shutdown time (6 runs) |
|---|---|---|---|
| without this PR | 43.0 s | 7 in 6 runs, all at 43 s | 7.96, 5.01, 5.05,
4.98, **28.3**, **27.9** s |
| with this PR | 23.0 s | 5 in 6 runs, all at 23 s | 9.12, 5.08, 5.00, 5.00,
5.00, 5.21 s |
The server still sometimes does not answer at 20 s. The client now gives up
at 23 s, the same bound as the Java client.
---
Tracked in #1412 (overview of the related Rust issues / PRs and a suggested
merge order).
--
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]
