hubcio commented on code in PR #4036:
URL: https://github.com/apache/iggy/pull/4036#discussion_r3928027765
##########
core/server/src/dispatch/mod.rs:
##########
@@ -615,45 +457,34 @@ async fn handle_client_request<B, MJ, S, SB>(
sessions.borrow_mut().record_heartbeat(transport_client_id);
let header = *request.header();
- if header.operation == Operation::NonReplicated {
- // Auth bypass guard: `PING`, the liveness probe, is the only pre-auth
- // code, on every roster shape. `GET_CLUSTER_METADATA` describes the
- // private replica network and is not something an unauthenticated
- // caller gets to read; a client that dialed a backup no longer needs
- // it to find the leader, because the backup authenticates the login
- // locally and forwards only the consensus proposal
- // (`submit_register_local_or_forward`). Every other non-replicated
- // code MUST go through Register first, which binds the acting user
- // the per-op authz gates resolve.
- let nr_code =
u32::from_le_bytes(request.header().reserved[..4].try_into().unwrap());
- // Legacy (pre-register) login codes. The server authenticates only via
- // the Register handshake (LOGIN_REGISTER / LOGIN_REGISTER_WITH_PAT,
- // Operation::Register); the vsr SDK funnels both logins there and
never
- // emits these. Reject them uniformly with a typed MalformedLogin (the
- // SDK maps it to InvalidFormat) before the session gate, so a legacy
or
- // foreign client fails fast instead of getting the generic
- // Unauthenticated deny the pre-auth guard would send unbound, or the
- // silent empty-ok Reply the bound non-replicated path would send.
- if matches!(
- nr_code,
- LOGIN_USER_CODE | LOGIN_WITH_PERSONAL_ACCESS_TOKEN_CODE
- ) {
+ let bound = sessions.borrow().get_session(transport_client_id);
Review Comment:
Both deferred items are in this PR after all, so the trade lands the other
way: the funnel prologue now costs fewer lookups than before the refactor, not
one more.
`SessionManager::touch_connection` stamps the heartbeat and returns the
bound session, the acting user and the peer address in ONE `connections` walk.
That replaces `record_heartbeat`, `get_session`, the `Partition` arm's
`get_user_id` and `reads.rs`'s `read_context`, and the `bus.client_meta` lookup
now runs only on a transport's first frame, which is the only place the address
has to come from the bus. `read_context` and the already-dead
`connection_address` are gone.
`header` is bound as a reference; only the `ReplicatedMetadata` arm takes
the by-value copy, which it needs for the pre-`transmute_header` echo on a
deny. `queues`, `active` and `SessionManager`'s two maps are on `ahash` now:
ids are server-minted and neither map is order-sensitive.
##########
core/server/src/dispatch/mod.rs:
##########
@@ -299,8 +230,19 @@ fn enqueue_client_request<B, MJ, S, SB>(
return;
}
- let bus = shard.bus.clone();
+ let shard_handle = Rc::clone(shard_handle);
+ let sessions = Rc::clone(sessions);
+ let system_config = Arc::clone(system_config);
+ let queues = Rc::clone(queues);
+ let active = Rc::clone(active);
bus.spawn(async move {
+ // The handle is set once the shard is built. A frame that beats
+ // it stays queued and the client's next frame drains both, so the
+ // active slot must be released here or that next frame never spawns.
+ let Some(shard) = upgrade_shard_handle(&shard_handle) else {
+ active.borrow_mut().remove(&client_id);
Review Comment:
The connection-lost hook now frees both `queues` and `active` for the
client, which covers the stranded queue entry and the
panic-leaves-the-slot-taken case in one place. `pop_next_client_request` keeps
the map entry when the queue drains empty, so a lockstep client no longer pays
a `HashMap` insert plus a `VecDeque` alloc/free per request, and the hook is
what removes it. Both maps moved to `ahash`.
The unbounded per-client `VecDeque` is deliberately NOT capped here. A cap
needs an overflow policy, whether to shed the frame, deny typed, or evict the
connection, plus an operator-facing knob to size it. Picking either silently
inside a review round is worse than leaving the queue as it is. I can open a
follow-up issue if you want it tracked.
##########
core/server/src/dispatch/session_ops.rs:
##########
@@ -552,35 +475,18 @@ async fn evict_stale_client<B, MJ, S, SB>(
if let Some((vsr_client_id, session)) = bound {
submit_disconnect_logout(Rc::clone(shard), vsr_client_id, session);
}
- let ctx = shard.plane.metadata().consensus.as_ref().map_or(
- consensus::EvictionContext {
- cluster: 0,
- view: 0,
- replica: 0,
- },
- consensus::EvictionContext::from_consensus,
- );
- let eviction = consensus::build_eviction_message(
- ctx,
+ warn!(
Review Comment:
Reworded rather than moved. `send_eviction` swallows the send result by
design, since it is best-effort and logs its own failure, so there is no result
left to branch on.
The line now claims only what it observed: the session was dropped and the
`Logout` submitted, and the notice is being sent. A delivery failure still
comes from `send_host_frame`, and it now carries the reason through the channel
label (see the `send_host_frame` thread).
--
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]
