atharvalade commented on code in PR #3108:
URL: https://github.com/apache/iggy/pull/3108#discussion_r3081791140
##########
core/metadata/src/impls/metadata.rs:
##########
@@ -311,9 +313,18 @@ where
return;
}
- let Some(_notify) = request_preflight(consensus, client_id,
request).await else {
- return;
+ // Register uses a dedicated preflight (check_register, request=0).
+ // Normal metadata ops use request_preflight (check_request with
session).
+ let preflight_ok = if operation == Operation::Register {
+ register_preflight(consensus, client_id).await.is_some()
+ } else {
+ request_preflight(consensus, client_id, session, request)
+ .await
+ .is_some()
};
+ if !preflight_ok {
+ return;
Review Comment:
`register_preflight(...).is_some()` gates progress, and `None` returns early
with no response to the client. Together with `register_preflight` behavior,
duplicate register requests will be dropped silently
##########
core/consensus/src/plane_helpers.rs:
##########
@@ -68,6 +75,48 @@ where
}
}
+/// Shared register preflight: duplicate detection for `Operation::Register`.
+///
+/// Returns `Some(Notify)` if the register is new and should proceed through
+/// consensus. Returns `None` if the client is already registered (session
+/// number sent back) or the register is already in progress.
+#[allow(clippy::future_not_send, clippy::unused_async)]
+pub async fn register_preflight<B, P>(
+ consensus: &VsrConsensus<B, P>,
+ client_id: u128,
+) -> Option<Notify>
+where
+ B: MessageBus<Replica = u8, Data = Message<GenericHeader>, Client = u128>,
+ P: Pipeline<Entry = PipelineEntry>,
+{
+ let status = consensus.client_table().borrow().check_register(client_id);
+ match status {
+ RequestStatus::AlreadyRegistered { session } => {
+ // Synthesize a register reply with the existing session.
+ // The caller can extract session from reply.header().commit.
+ tracing::debug!(
+ client_id,
+ session,
+ "register_preflight: client already registered, ignoring"
+ );
+ None
+ }
+ RequestStatus::InProgress => None,
Review Comment:
on `AlreadyRegistered`, the code returns `None` without sending the
existing-session reply it says it will synthesize. This can black-hole
duplicate `Register` retries (lost reply/timeout path), leaving clients stuck
waiting instead of getting a deterministic session response.
--
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]
