numinnex commented on code in PR #3753:
URL: https://github.com/apache/iggy/pull/3753#discussion_r3658642751
##########
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:
Correcting myself on both counts here: I declined the remedy, and I was
wrong. It has landed.
`commit_register` now sets the entry's epoch to the register's commit op
(`reply.header().commit`, stamped from the prepare's op by
`build_reply_message`) instead of `1, +1 per rebind`.
What I missed when I argued for the counter: a counter resets. Drop an entry
by capacity eviction and re-register the same key, and the fresh entry is back
at epoch 1 -- indistinguishable from the zombie still holding epoch 1 from
before the eviction, so the fence is blind exactly where it is most needed. An
op-derived fence cannot regress, because ops do not. That is your "log-derived
identity" point, and it is the property the counter was quietly trading away.
My "not comparable across planes" objection conflated the fence *value* with
the table's *identity*. The key stays client-supplied; only the fence is
op-derived. Register commits solely in the metadata group, so there is exactly
one minting authority and the value is a plain `u64` for any group's slice to
store and compare. Our own design note said as much before the counter drifted
in ("Register is always a metadata-plane op, so there is exactly one minting
authority. Monotonic within the metadata group = globally comparable
everywhere") -- I argued against a decision that had already been made
correctly.
The inertness half of your diagnosis is fixed alongside it, because deriving
the value changes nothing if no bind ever commits. `submit_register_in_process`
lost its fast-path short-circuit and `register_preflight` no longer consults
the table (`check_register` and `RequestStatus::AlreadyRegistered` are
deleted), so every bind proposes. The ownership gate stays where it was and
still refuses another user's entry before dispatching; an owned entry now falls
through to a real Register, whose rebind branch refences the entry while
preserving its watermark and reply ring. `Fenced`/`EpochAhead` are reachable on
the live path for the first time.
One thing that falls out for free: the reconnect-versus-reclaim-sweep race
is closed. A sweep's in-flight `Logout` carries the pre-rebind epoch, and
`submit_logout_in_process`'s epoch guard drops it rather than tearing down the
session the client just rebound.
Cost is a consensus round trip per reconnect, which is the honest price of a
bind being a logged event.
##########
core/consensus/src/client_table.rs:
##########
@@ -412,9 +482,9 @@ impl ClientTable {
/// state -> identical choice. `commit_journal` catch-up has empty
pipeline,
Review Comment:
Withdrawing the second half of this reply. The determinism fix and the
provenance note stand; the "it is an improvement, not a regression" argument
does not, and it was reasoning from the wrong place.
The trace I gave was accurate as far as it went: master takes the "newer
than issued (client bug)" arm and silently drops, this branch produces `Fenced`
-> terminal eviction, and an eviction does recover faster than a read timeout.
But comparing failure modes was the wrong exercise. The epoch was not supposed
to be *diverging* on either branch. I used master's worse handling of a bad
state to argue our bad state was fine, instead of asking why the state was
reachable at all -- which is what your `client_table.rs:370` comment was
pointing at, and which I also pushed back on there.
The actual answer is that a per-entry counter was the wrong derivation, and
it is now the register's commit op (see the reply on client_table.rs:370).
Under that scheme the spurious-fence case I was defending cannot arise: epochs
come from log positions, so replicas that agree on the log agree on every
entry's fence, whether or not they made the same local eviction decision. There
is no divergent-epoch state left to characterize as better or worse.
The `in_flight` removal you asked for is what makes the victim choice agree
in the first place, and that part shipped as described.
--
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]
