numinnex commented on code in PR #3828:
URL: https://github.com/apache/iggy/pull/3828#discussion_r3730090617
##########
core/server-ng/src/dispatch.rs:
##########
@@ -724,6 +730,80 @@ fn pop_next_client_request(
message
}
+/// Per-request partitions-count cap shared by create-topic, create-partitions
+/// and delete-partitions admission. Runs pre-consensus like
+/// [`validate_topic_bounds`]: an oversized count must not burn a replicated
+/// log entry (create-partitions admission would also allocate that many
+/// consensus-group ids before replicating).
+pub(crate) const fn validate_partitions_count(partitions_count: u32) ->
Result<(), IggyError> {
+ if partitions_count > MAX_PARTITIONS_PER_REQUEST {
Review Comment:
The cap is checked but zero is not, and legacy TCP rejects zero on both
partitions ops:
`core/server/src/binary/handlers/partitions/create_partitions_handler.rs:43`
(`!(1..=MAX)`) and `delete_partitions_handler.rs:42` (`== 0`). HTTP already
rejects it too (`core/common/src/http/partitions/create_partitions.rs:53`,
`delete_partitions.rs:53`), so this also splits TCP from HTTP within server-ng,
and the new test only probes `MAX+1`.
It is not a harmless no-op reply on the create side: `create_partitions(0)`
passes validation, `allocate_many(0)` yields an empty vec, and
`CreatePartitionsWithAssignmentsRequest::apply` bumps `state.revision`
unconditionally and calls `rebalance_consumer_groups()`
(`core/metadata/src/stm/stream.rs:1838-1858`). Each such request burns a
replicated log entry, forces a consumer-group rebalance, and disarms the
reconciler fast-skip into a full O(N) diff plus truncation and purge sweeps on
every shard — client-driven amplification from a call that creates nothing.
`delete_partitions(0)` is a clean no-op by comparison (`:1884-1898`), costing
one wasted log entry.
Suggest `1..=MAX_PARTITIONS_PER_REQUEST` for both partitions ops, keeping
`0..=MAX` for create_topic since legacy allows it there
(`create_topic_handler.rs:47`).
Also worth folding in while this area is open: the typed `PartitionNotFound`
this PR adds is TCP-only. HTTP poll collapses it into a generic
`ResourceNotFound` (`http/handlers.rs:1041`) where legacy HTTP propagates the
typed error with a `partition_id` field
(`core/server/src/http/error.rs:50,90`); `get_consumer_offset` still replies an
empty body for a bad partition id (`:1971-1978`) where legacy returns
`PartitionNotFound` (`core/server/src/shard/system/consumer_offsets.rs:124`);
and send / store|delete-consumer-offset / delete-segments hardcode
`ResourceNotFound` at `:1218`, discarding the resolver's typed error, where
legacy returns 3007. The last two are pre-existing, but they are the same
"empty success a consumer reads as end-of-data" class this PR set out to close.
##########
core/server-ng/src/responses.rs:
##########
@@ -1193,13 +1176,18 @@ fn partition_response(
// across all shards and both left-right buffers), populated when the
// owning shard materializes the partition; `None` only in the window
// before that first materialization.
+ //
+ // A committed partition always materializes with exactly one empty
+ // segment, so before the owning shard gets there (registry miss, or
+ // registered but not yet segmented) the reply reports that deterministic
+ // initial state instead of a zero a client would read as "no storage".
let stats = streams
.stats_registry
.partition_get(stream_id, topic_id, partition.id);
let (segments_count, current_offset, size_bytes, messages_count) =
- stats.map_or((0, 0, 0, 0), |stats| {
+ stats.map_or((1, 0, 0, 0), |stats| {
(
- stats.segments_count_inconsistent(),
+ stats.segments_count_inconsistent().max(1),
Review Comment:
The `.max(1)` on the materialized read hides a real zero. A partition whose
owning shard actually has zero segments — a failed `install_empty_segment`
inside `purge()` (`core/partitions/src/iggy_partition.rs:3141-3171`) leaves
exactly that state, and it is also the state that then panics the shard via the
`segments().len() - 1` underflow at `:1008` or `active_segment()`'s `expect` at
`core/partitions/src/log.rs:338` — reports as one healthy empty segment. That
is the one case where monitoring most needs the truth. Legacy never fabricates
the count (`core/server/src/binary/handlers/topics/get_topic_handler.rs:64`);
it just happens to be 1 because create increments synchronously.
The registry-miss `1` on the line above is more defensible as the
deterministic committed state, but the comment at `:1180-1183` is factually
wrong about when it applies: `partition_response` iterates the metadata STM,
whose row outlives a shard-side tombstone and a mid-teardown, so the miss
window is not only "before that first materialization" — a torn-down partition
also reports one empty segment.
Suggest dropping `.max(1)` and correcting the comment. Worth noting the
fabricated value silently defeated the suite's only materialization check,
whose convergence loop this PR removed
(`core/integration/tests/server/scenarios/system_scenario.rs:175`).
--
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]
