Improve batch publish errors & bump dependencies#23
Conversation
Signed-off-by: Tomasz Pietrek <tomasz@synadia.com>
|
Claude finished @Jarema's task in 2m 10s —— View job PR Review
SummaryThe error-handling additions ( One blocking issue and two minor ones: Blocking — missing Minor — hard-coded server limit in error messages Minor — stale test name |
![synadia-claude-reviewer[bot]](https://avatars.githubusercontent.com/u/132956691?s=60&v=4){kind=small}
|
|
||
| pub mod batch_fetch; | ||
| pub mod batch_publish; | ||
| pub mod batch_publish_fast; |
There was a problem hiding this comment.
Build-breaking: batch_publish_fast.rs is missing.
pub mod batch_publish_fast; is declared here, and the README/crate-level docs reference jetstream_extra::batch_publish_fast::FastPublishExt, but no batch_publish_fast.rs (or batch_publish_fast/mod.rs) file exists in the repository. The crate will not compile as-is. The file needs to be included in this PR, or the module declaration and its documentation should be deferred to the follow-up PR that implements the module.
| BatchPublishNotEnabled, | ||
| /// Batch publish is incomplete and was abandoned | ||
| BatchPublishIncomplete, | ||
| /// Server has too many inflight batches (server limit: 50) |
There was a problem hiding this comment.
The server limit of 50 is hard-coded in both the doc comment and the Display impl below. If the NATS server changes this limit in a future release the error message will be stale. Consider either omitting the specific number or noting it as the value at the time of the async-nats 0.48 / nats-server 2.14 release:
| /// Server has too many inflight batches (server limit: 50) | |
| /// Server has too many inflight batches | |
| BatchPublishTooManyInflight, |
| Self::BatchPublishIncomplete => { | ||
| write!(f, "batch publish is incomplete and was abandoned") | ||
| } | ||
| Self::BatchPublishTooManyInflight => { |
There was a problem hiding this comment.
Same hard-coded limit concern as the doc comment above. Suggestion:
| Self::BatchPublishTooManyInflight => { | |
| Self::BatchPublishTooManyInflight => { | |
| write!(f, "server has too many inflight batches") | |
| } |
| err.unwrap_err().kind(), | ||
| BatchPublishErrorKind::BatchPublishIncomplete | ||
| err.kind(), | ||
| BatchPublishErrorKind::BatchPublishTooManyInflight |
There was a problem hiding this comment.
The assertion flip from BatchPublishIncomplete to BatchPublishTooManyInflight is correct for this test scenario (51st concurrent batch hitting the server inflight limit), but the test name test_incomplete_batch_error no longer matches what it actually tests. Consider renaming it to test_too_many_inflight_error to avoid confusion.
1 participant
Signed-off-by: Tomasz Pietrek tomasz@synadia.com