github-actions[bot] commented on code in PR #4387:
URL: https://github.com/apache/iggy/pull/4387#discussion_r4174005822
##########
core/sdk/src/session.rs:
##########
@@ -98,66 +86,45 @@ impl ConsensusSession {
self.session.is_some()
}
- /// Bind the session after register commits through consensus.
- /// The `session` value is the commit op number from the server's reply.
- ///
- /// # Panics
- /// Panics if already bound (drop and create a new session instead).
- pub fn bind(&mut self, session: u64) {
- assert!(
- self.session.is_none(),
- "session already bound (session={})",
- self.session.unwrap()
- );
- assert!(session > 0, "session must be > 0");
+ /// Registration proof retained across reconnects and session bindings.
+ pub fn bind_secret(&self) -> BindSecret {
+ self.bind_secret.clone()
+ }
+
+ /// Accept the original registration, including its replay after reconnect.
+ pub fn bind(&mut self, session: u64) -> Result<(), IggyError> {
+ if session == 0 || self.session.is_some_and(|bound| bound != session) {
Review Comment:
warning: `bind()` reports a conflicting nonzero epoch as `InvalidSession`,
whose message says the value must be non-zero, so the caller reads the wrong
cause. Return `SessionMismatch(requested, bound)` for a conflict, and give the
counter-overflow arm its own error.
##########
foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/vsr/VsrLoginCodec.java:
##########
@@ -38,12 +38,13 @@
final class VsrLoginCodec {
/**
- * Packed semver of the {@code iggy_binary_protocol} crate this codec
- * targets: {@code major << 20 | minor << 10 | patch}, 10 bits per field.
- * Keep in sync with {@code core/binary_protocol/Cargo.toml}; the server
- * accepts any client whose major.minor is not newer than its own.
+ * Packed wire protocol version this codec targets:
+ * {@code major << 20 | minor << 10 | patch}, 10 bits per field.
+ * Keep in sync with {@code core/binary_protocol/src/version.rs},
independently
+ * of the crate release. The server enforces its minimum wire version and
rejects
+ * newer major.minor versions.
*/
- static final int PROTOCOL_VERSION = (11 << 10); // 0.11.0
+ static final int PROTOCOL_VERSION = (11 << 10) | 1; // 0.11.1
Review Comment:
nit: The Java codec pins the packed protocol version by hand while Go, C#,
and Node check theirs against `core/binary_protocol/src/version.rs`. Add a Java
test that reads the Rust constant, so a bump cannot leave Java clients rejected
at login.
##########
core/sdk/src/clients/producer.rs:
##########
@@ -61,17 +61,10 @@ pub(crate) fn no_confirmations() -> SendMessagesResponse {
}
}
-/// Why `error` ends the retries of a send, or `None` when the producer's
-/// at-least-once retry policy applies.
-const fn send_retry_stop_reason(error: &IggyError) -> Option<&'static str> {
- match error {
- // Raised while decoding an HTTP reply body, after a successful status.
- IggyError::InvalidBytesResponse | IggyError::InvalidJsonResponse => {
- Some("batch committed, confirmation unreadable")
- }
- IggyError::RequestTooOld => Some("outcome unknown, a later write
already committed"),
- _ => None,
- }
+/// A producer retry creates another request identity. Only an explicit
+/// non-admission permits that; ambiguous retries belong to the transport.
+const fn stops_send_retries(error: &IggyError) -> bool {
Review Comment:
nit: HTTP transport errors never match `TransientNotAccepted`, so
`send_retries` never retries a send over HTTP, and the retry table does not say
so. Add one line naming the HTTP transport exception.
##########
core/journal/src/partition_journal/segments.rs:
##########
@@ -666,6 +720,9 @@ impl<S: DurableStorage> PartitionPrepareJournal<S> {
self.segment_files_dirty = true;
self.segment_links_dirty = true;
self.segment_files.insert(key, file);
+ // Establish the inode and links before buffered appends can be
acknowledged.
+ // A later shutdown barrier must not need a new directory descriptor.
+ self.sync_segment_files().await?;
Review Comment:
nit: `open_segment_file` syncs every open segment file and both directories
even when the retained file already existed and no link was created. Set the
dirty flags and sync only in the branch that creates the file or link.
##########
core/message_bus/src/replica/handshake.rs:
##########
@@ -149,10 +152,8 @@ pub async fn acceptor_handshake<S: AsyncRead + AsyncWrite>(
let header = msg.header();
let peer_id = header.replica;
let has_nonce = auth::has_nonce(&header.reserved_command);
- // Only a peer that sent a nonce speaks the authenticated protocol and is
- // waiting to read our response; a legacy (no-nonce) dialer delegates its
- // fd without reading, so a reject frame would land in its VSR reader
instead.
- let nackable = auth.is_some() && has_nonce;
+ // Both authenticated and unauthenticated peers read the build-gate
response.
+ let nackable = true;
Review Comment:
simplification: `nackable` is now the constant `true`, and the
`AuthRequired` reject passes `false` directly. Delete the binding and pass
`true` at the four call sites. Saves about two lines and one misleading local.
--
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]
