leizhiyuan opened a new pull request, #1427:
URL: https://github.com/apache/rocketmq-clients/pull/1427
### Which Issue(s) This PR Fixes
Fixes #1426
### Brief Description
**Scenario.** The batch size and the invisible duration passed to
`SimpleConsumer` are usually computed or read from configuration. When a bug or
a misconfiguration yields `0`, a negative value or a huge duration, the caller
should get an error back. Today:
- `receive_with(.., -1, ..)` **panics** with `capacity overflow`
(`Vec::with_capacity(batch_size as usize)` in `Client::receive_message_inner`).
- `receive_with(.., 0, ..)` is sent to the server and comes back as `Ok([])`
after the full long polling time.
- `receive_with(.., Duration::MAX)` and `change_invisible_duration(..,
Duration::MAX)` **panic** in `prost_types::Duration::try_from(..).unwrap()`.
**Change.** This aligns with Java, where `SimpleConsumerImpl#receive0` fails
with `IllegalArgumentException("maxMessageNum must be greater than 0")` before
any RPC.
- `SimpleConsumer::receive_with` returns `ErrorKind::Config`, "batch size
must be greater than 0" (with `batch_size` in the context), when `batch_size <=
0`. No route lookup or RPC happens.
- `receive_with` and `change_invisible_duration` return `ErrorKind::Config`,
"invisible duration is out of range", instead of panicking when the duration
can not be represented in protobuf. The `unwrap()`s are removed.
- `ErrorKind::Config` is what the crate already uses for invalid caller
input (e.g. an empty consumer group).
#### Notes for reviewers
- **Scope.** Only `rust/src/simple_consumer.rs` changes. `PushConsumer` is
not affected: its batch size comes from `PushConsumerOption`, which has no
public setter, and its invisible duration conversion already maps the error.
- **Conflicts.** `git merge-tree` is clean against #1404, #1407, #1409,
#1411, #1414, #1416, #1420, #1421 and #1425. It conflicts with #1381 in
`simple_consumer.rs`, because #1381 moves the body of `receive_with` into a
shared `receive_internal` (used by `receive_with` and `LiteSimpleConsumer`).
The resolution is to put the two checks at the top of `receive_internal`, so
they also cover `LiteSimpleConsumer::receive`. I verified it on a temporary
merge of #1381 + this PR: `cargo test --lib` 100 passed. #1423 shows the same
conflict only because it carries #1381. Overview in #1412.
- **Behavior change.** `receive_with(.., 0, ..)` used to return `Ok([])`
after a long poll and now returns an error immediately. A zero batch size can
never return a message, so no working usage is affected.
### How Did You Test This Change?
**Unit tests.** All three fail on `master` (two hit the mock's `never()`
because the RPC is actually issued, one panics in the `unwrap()`):
- `receive_with_rejects_non_positive_batch_size`: `0`, `-1`, `i32::MIN`.
- `receive_with_rejects_out_of_range_invisible_duration`: `Duration::MAX`.
- `change_invisible_duration_rejects_out_of_range_invisible_duration`:
`Duration::MAX`.
The mocked client expects no `topic_route`, `receive_message` or
`change_invisible_duration` call.
```
$ cargo test -- --test-threads=1
test result: ok. 92 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
$ RUSTDOCFLAGS='-D warnings' cargo doc --no-deps # no warnings
$ cargo build --examples --all-features # ok
```
`cargo clippy --all-targets` reports the same findings as `master`. With the
default parallel test runner, `client::tests::client_get_session` failed once
out of several runs; it is an existing flaky test on `master` (it uses
`MockSession::new_context()` without holding `MTX`) and is unrelated to this
change. It passed in 60 out of 60 runs of the test binary both with and without
this change.
**End-to-end.** Against a managed RocketMQ 5 server, with a normal topic and
group:
| call | before | after |
|---|---|---|
| `receive_with(.., 0, 10s)` | `Ok([])` after 20 s | `Err(Config: batch size
must be greater than 0)` in 34 µs |
| `receive_with(.., -1, 10s)` | panic `capacity overflow` | same error, 17
µs |
| `receive_with(.., i32::MIN, 10s)` | panic | same error |
| `receive_with(.., 1, Duration::MAX)` | panic | `Err(Config: invisible
duration is out of range)` |
| `receive_with(.., 1, 10s)` | `Ok` | `Ok` (unchanged, 20 s long poll) |
---
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]
