numinnex commented on code in PR #3753:
URL: https://github.com/apache/iggy/pull/3753#discussion_r3658027139
##########
core/consensus/src/client_table.rs:
##########
@@ -412,9 +482,9 @@ impl ClientTable {
/// state -> identical choice. `commit_journal` catch-up has empty
pipeline,
Review Comment:
The determinism claim was wrong exactly as you describe, and the `warn!`
point is fair too. Fixed by deleting `in_flight` from `evict_oldest` and from
`commit_register`'s signature rather than by caching the header fields: the
victim is now oldest-commit with a lowest-slot tiebreak and nothing local feeds
the choice. The doc now states the constraint positively, that pipeline state
must not be consulted because only a primary populates it. A `warn!` also comes
back on the other side of the same divergence, `commit_reply` now returns
`SkippedRegression { stored, received }` and the caller logs it.
Two provenance corrections though.
`in_flight`, its two queue scans and the "pipeline state derives from the
agreed log" sentence are all present on master, and `evict_oldest` diffs
semantically clean against it. So the divergence is not introduced by this PR.
Worth fixing here since the PR is what makes the table durable, but it is not a
regression.
The consequence you draw from it is also better under this PR than before
it, not worse. Take the same primary/backup split and follow it to what the
client sees:
- master: the backup takes `commit_register`'s session-mismatch arm, skips
the update and keeps `session = A`, while the client holds the newer `M` from
the primary. On failover `check_request` sees `expected(A) < received(M)`,
which is the "newer than issued, client bug" arm: error log, no eviction, no
reply. The client waits out its read timeout before it learns anything.
- here: the backup bumps to epoch 2 while the client holds 1. On failover
that is `Fenced`, which maps to a terminal `Evict(SessionTooLow)`. Eviction
tells the client to re-register, so it recovers on the next round trip.
Same divergence, and the fence turns a silent wedge into a frame that names
the problem. So the spurious-fence symptom is new, but it is an improvement
over the silent drop it replaced.
##########
core/consensus/src/client_table.rs:
##########
@@ -260,55 +322,44 @@ impl ClientTable {
reply.header().client
);
- let session = reply.header().commit;
- assert!(session > 0, "commit_register: session must be > 0");
-
- let existing = self.index.get(&client_id).copied();
-
- // Mismatch on re-register: log + skip, not panic. See doc above.
- if let Some(slot_idx) = existing {
- let slot = self.slots[slot_idx].as_ref().expect("index/slot
mismatch");
- if slot.session != session {
- tracing::warn!(
- client_id,
- existing_session = slot.session,
- replay_session = session,
- "commit_register: session mismatch (stale WAL replay or \
- duplicate Register at different ops); skipping update"
- );
- return;
- }
- }
-
// Freeze once; later dedup-hit clones Arc-bump.
let cached: CachedReply = CachedReply::from_message(reply);
- // Update in place on re-register, else new slot. Reply-delivery
- // channel lives on popped `PipelineEntry`, fired by commit caller
- // after this returns, slot-first ordering, see `commit_reply`.
- if let Some(slot_idx) = existing {
- self.slots[slot_idx]
- .as_mut()
- .expect("index/slot mismatch")
- .reply = cached;
+ if let Some(&slot_idx) = self.index.get(&client_id) {
+ let entry = self.slots[slot_idx].as_mut().expect("index/slot
mismatch");
+ entry.epoch += 1;
+ entry.user_id = user_id;
+ // Drop the previous register reply (if still retained) before
+ // pushing the new one: only the newest rebind's reply is
+ // replayable, and two request-0 entries would break the ring's
+ // unique-request invariant.
+ entry
+ .ring
+ .retain(|stored| stored.header().request !=
REGISTER_REQUEST_ID);
+ entry.push_latest(cached);
} else {
if self.index.len() >= self.slots.len() {
self.evict_oldest(&in_flight);
}
let slot_idx = self.first_free_slot().expect("eviction must free a
slot");
+ let mut ring = VecDeque::with_capacity(REPLY_RING_CAPACITY);
+ ring.push_back(cached);
self.slots[slot_idx] = Some(ClientEntry {
- session,
+ epoch: 1,
Review Comment:
Diagnosis accepted, remedy declined.
The two observations hold. A fresh entry does reset epoch and watermark, and
unlogged entry drops leave nothing in the log to reconstruct what the entry
held. The fence is inert on the live path for the reason you give: the register
fast path returns before proposing, so nothing bumps an epoch past 1 and
`Fenced`/`EpochAhead` are unreachable there.
Making the fence the register's op number is the part I do not want. The
table is deliberately free of any value derived from a commit op number,
because one logical table spans both the metadata plane and the partition plane
and those have independent op ranges. An op-derived fence is not comparable
across them, so it buys log-derived identity for the metadata plane at the cost
of the table no longer being one mechanism. That constraint is the reason
`session` stopped being `prepare_header.op` in the first place, so restoring it
is a design reversal rather than a fix.
The alternative that keeps the model is to make rebind actually propose a
Register, i.e. delete the fast-path short-circuit, so the bump is real, logged
and replica-identical without anything in the entry deriving from an op number.
That is the change I would rather make, and it is not in this PR.
The "fixes the replica-divergence problem below at the same time" half is
moot now regardless: the divergence is fixed at its source by removing pipeline
state from the victim choice (see the reply on client_table.rs:482), so an
op-derived fence is no longer load-bearing for it.
##########
core/metadata/src/impls/metadata.rs:
##########
@@ -995,9 +1011,12 @@ where
.as_ref()
.expect("submit_register_in_process: consensus only exists on
shard 0");
- // Idempotent fast path: existing session skips pipeline + wire-reply.
- if let Some(session) =
self.client_table.borrow().get_session(client_id) {
- return Ok(session);
+ // Idempotent fast path: existing entry skips pipeline + wire-reply.
+ // Returns the current epoch without bumping it; rebind-bumps-epoch
+ // (zombie fencing per reconnect) arrives with the stable-session-id
+ // auth flow, which commits a Register per bind.
+ if let Some(epoch) = self.client_table.borrow().get_epoch(client_id) {
Review Comment:
Fixed: the fast path now requires the authenticated user to own the entry,
and a mismatch is a terminal `MetadataSubmitError::ClientIdOwnedByAnotherUser`
so the SDK fails hard instead of spinning on it. The resume path that shared
the hole is deleted outright rather than gated.
Two corrections to the framing.
The mechanism is not introduced here. Master's fast path is the same early
return with no owner check (`get_session` where this branch reads `get_epoch`),
and `resolve_acting_user_id` is byte-identical on master. What this PR changes
is reachability, not the missing check: recovery now rebuilds the table across
a restart, which is what makes the collision you describe on recovery.rs:220 a
routine event rather than a nonexistent one. Both still needed fixing here,
since this is the PR that makes them reachable.
The scope paragraph understates it in your own favour. "partition ops
resolve the user from `SessionManager` instead, so those stay on the attacker's
own identity" is true for the fast path, but false for resume:
`try_resume_session` called `SessionManager::login` with the `user_id` it read
out of the table, so `dispatch_partition_request` resolved the victim's
identity too. The data plane was in scope for that path, not just replicated
metadata ops.
--
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]
