hubcio commented on code in PR #4024:
URL: https://github.com/apache/iggy/pull/4024#discussion_r3929122457
##########
core/server/src/http/submit.rs:
##########
@@ -106,13 +105,32 @@ pub(in crate::http) async fn submit_committed(
let (result_slot, committed) = oneshot::channel();
let shard = Rc::clone(&state.shard);
let task_session = Rc::clone(session);
+ let watermarks = Rc::clone(&state.metadata_watermarks);
let body = body.to_vec();
let max_tokens_per_user = state.max_tokens_per_user;
// Detached so a client disconnect cannot abandon the gate mid-submit;
// the write runs to completion regardless of handler liveness.
compio::runtime::spawn(async move {
let result =
submit_gated(&shard, &task_session, operation,
max_tokens_per_user, &body).await;
+ // Recorded here rather than after the await below, for the same reason
+ // the submit is detached: a caller that disconnected mid-write still
+ // committed the op, and its next request as this user must not be
+ // served state older than what committed. Ordered before the wake, so
a
+ // read issued the instant the response lands already sees the mark.
+ //
+ // This node's own view only: on a follower with HTTP forwarding ON the
+ // write is relayed to the primary by the middleware and never reaches
+ // this task, so the follower's floor stays where the register left it
+ // and its read-your-writes guarantee is the register epoch's. Closing
Review Comment:
the last three lines are stale as of this branch. `forward.rs` now copies
`iggy-applied-op` off the relayed response and `record_relayed_floor` records
it as this caller's floor, so the primary's commit op does come back with the
relayed response. a future reader lands here and concludes the forwarding hole
is still open.
one caveat that does survive and is worth keeping in the rewrite:
`classify_reply` relays a 503 carrying `TransientNotCommitted` instead of
retrying it, and `record_relayed_floor` only records on 2xx - so a write that
did commit behind that code still leaves no floor. that's the honest version of
what this comment is reaching for.
##########
core/server/src/http/reads.rs:
##########
@@ -117,6 +119,108 @@ pub(in crate::http) async fn read_local(
}
}
+/// Every gate a read served from THIS node's state has to pass, in the one
+/// order that is safe.
+///
+/// The chokepoint for the whole REST read surface: [`read_local`] runs it for
+/// the metadata-STM entity reads, and the routes that cannot use `read_local`
+/// call it directly - the cross-shard client reads, which serve from each
+/// shard's session manager; the snapshot route, which shells out; and the
+/// partition reads, which answer from a partition group's log. Skipping it is
+/// how a route silently loses authorization, the post-restart barrier, or the
+/// read-your-writes hold; which of the two waits actually applies is
+/// [`read_needs_metadata_frontier`]'s decision, not the caller's.
+///
+/// Order:
+/// 1. the recovery barrier, so nothing is served off a WAL suffix that is
+/// about to re-commit;
+/// 2. authorization, because it is terminal: the linearizable follower
+/// redirect must answer 307 immediately rather than after a park, and
+/// holding a connection to then answer 403 buys nothing;
+/// 3. the read-your-writes hold;
+/// 4. authorization AGAIN if that hold actually parked. Every scoped route's
+/// rule resolves its entity when the rule RUNS, and a park is precisely
+/// the case where the state machine moved under it: an entity that did not
+/// exist on the first pass resolved to nothing, where a scope miss is a
+/// pass-through, and would be served with no permissioner call at all. Only
+/// the parked outcome pays for the second pass.
+pub(in crate::http) async fn gate_local_read(
+ state: &HttpInner,
+ identity: &Identity,
+ consistency: Consistency,
+ code: u32,
+ rule: impl Fn(&Permissioner, u32) -> Result<(), IggyError>,
+) -> Result<(), ReadError> {
+ await_recovery_barrier(&state.shard).await?;
+ authorize_read(state, identity, consistency, &rule)?;
+ if read_needs_metadata_frontier(code)
+ && await_metadata_read_frontier(state, identity).await? ==
FrontierWait::CaughtUp
+ {
+ authorize_read(state, identity, consistency, &rule)?;
+ }
+ Ok(())
+}
+
+/// Hold a local metadata read until this node has applied everything the
+/// calling user was told committed.
+///
+/// A committed control-plane reply hands the caller an op number; answering
its
+/// next read from a state machine below that op contradicts the response it is
+/// holding. The lag is real on a node that is not the metadata primary: a
+/// healthy backup FORWARDS a `Register` to the primary
+/// (`dispatch::submit_register_local_or_forward`) and binds the committed
epoch
+/// while its own commit walk is still behind it, so the caller holds an op
that
+/// node has not applied before it has issued a single write. That is the whole
+/// window on a backup: a control-plane write posted there is either relayed to
+/// the primary (forwarding on) or refused transient (forwarding off), so the
+/// epoch is the only promise a backup makes on its own.
+///
+/// Adjacent to `?consistency=linearizable`, not in competition with it. That
+/// asks for the freshest CLUSTER state and is answered by leaving this node
+/// (307 to the primary), which [`authorize_read`] decides before this wait and
+/// which this wait never sees. This gate makes an UNQUALIFIED read
+/// read-your-writes for its own user, at no redirect and no consensus round
+/// trip. Per user rather than per credential because a bearer is not stable:
+/// a refreshed access token is a new credential for the same writer (see
+/// [`crate::http::state::MetadataWatermarks`]).
+///
+/// Scope is this node's own view. A user whose write this node relayed over
+/// HTTP, or who wrote through a different node entirely, left no floor here;
+/// closing that needs the serving primary's commit op to reach the reading
Review Comment:
first clause is stale - a write this node relayed over HTTP does leave a
floor now, `record_relayed_floor` records the serving primary's applied op off
the response header. second clause still holds: a user who wrote through a
different node entirely leaves nothing here.
something like:
```
/// Scope is this node's own view. A write this node RELAYED leaves a floor
/// here: the relay records the serving primary's applied op off the response
/// (see `http::forward::record_relayed_floor`). A user who wrote through a
/// different node entirely still leaves none.
```
--
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]
