krishvishal commented on code in PR #3223:
URL: https://github.com/apache/iggy/pull/3223#discussion_r3231867083
##########
core/server-ng/src/login_register.rs:
##########
@@ -17,71 +17,74 @@
* under the License.
*/
-//! Combined login + register handler for server-ng.
+//! Combined login + register handler.
//!
-//! One client-facing command, two internal phases:
-//! 1. Verify credentials locally (Argon2). NOT through consensus
-//! 2. Submit `Operation::Register` through consensus. All replicas create
`ClientTable` entry
+//! One client command, two phases:
+//! 1. Local credential verify (Argon2). Not consensus.
+//! 2. `Operation::Register` through consensus -> `ClientTable` entry on all
replicas.
//!
-//! The handler is trait-based so it can be tested via mocking.
+//! Trait-based for mocking.
use crate::session_manager::{SessionError, SessionManager};
+use iggy_binary_protocol::EvictionReason;
use iggy_binary_protocol::requests::users::{LoginRegisterRequest,
LoginRegisterWithPatRequest};
use iggy_binary_protocol::responses::users::LoginRegisterResponse;
+use metadata::RegisterSubmitError;
use secrecy::ExposeSecret;
-/// Credential verification abstraction.
-///
-/// The real implementation delegates to the shard's metadata user store
-/// and Argon2 password hashing. Test implementations return fixed results.
+/// Credential verifier. Real impl: metadata user store + Argon2.
pub trait CredentialVerifier {
- /// Verify username/password. Returns `user_id` on success.
+ /// Verify username/password. Returns `user_id`.
///
/// # Errors
- /// Returns `LoginRegisterError` if credentials are invalid.
+ /// `LoginRegisterError` on invalid credentials.
fn verify(&self, username: &str, password: &str) -> Result<u32,
LoginRegisterError>;
}
-/// Personal access token verification abstraction.
-///
-/// The real implementation looks up the PAT by hash in the user store,
-/// checks expiry, and returns the owning user's ID.
+/// PAT verifier. Real impl: hash lookup + expiry check.
pub trait TokenVerifier {
- /// Verify a personal access token. Returns `user_id` on success.
+ /// Verify PAT. Returns `user_id`.
///
/// # Errors
- /// Returns `LoginRegisterError` if the token is invalid or expired.
+ /// `LoginRegisterError` on invalid/expired token.
fn verify_token(&self, token: &str) -> Result<u32, LoginRegisterError>;
}
-/// Consensus register submission abstraction.
+/// Submit `Register` through consensus.
+///
+/// # Runtime
+/// `Future` is **not `Send`**. Production
+/// ([`crate::register_submitter::IggyRegisterSubmitter`]) wraps
+/// `IggyMetadata::submit_register_in_process`, whose state is `RefCell`/`Cell`
+/// on single-threaded `compio`. Multi-threaded embedders need a shim or
+/// custom transport; constraining `Send` would tax every call site.
///
-/// The real implementation builds a `RequestHeader { operation: Register }`,
-/// calls `check_register` on the `ClientTable`, submits through the consensus
-/// pipeline, and awaits the `Notify` for commit. Returns the session number
-/// (commit op number).
+/// # Failures
+/// Transient (pipeline-full, view-change cancel, primary-not-caught-up,
+/// in-flight register) MUST eventually be absorbed via bounded retry. Until
+/// then they surface as [`LoginRegisterError::Transient`] → `NotEvictable`,
+/// so network can't ship a wire-terminal `Eviction` for a recoverable
+/// failure. SDK read-timeout replays.
+///
+/// Terminal → [`EvictionReason`] in `EvictionHeader`; SDK invokes its
+/// eviction callback and stops.
pub trait RegisterSubmitter {
Review Comment:
Pushed a commit addressing this feedback. Dropped the trait and calls
`IggyMetadata::submit_register_in_process` directly
--
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]
