ryerraguntla commented on code in PR #4043: URL: https://github.com/apache/iggy/pull/4043#discussion_r4020287040
########## gateways/kafka/src/bridge/iggy_bridge.rs: ########## @@ -0,0 +1,503 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::future::Future; +use std::time::Duration; + +use iggy::prelude::{ + AutoLogin, Client, Credentials, Identifier, IggyClient, IggyClientBuilder, IggyError, + StreamClient, TopicClient, TopicCreateOptions, +}; +use tracing::{debug, info}; + +use crate::bridge::config::IggyBridgeConfig; +use crate::bridge::error::BridgeError; +use crate::bridge::topic_map::validate_kafka_topic_name; + +/// Passes attempted, after the first, before [`IggyBridge::connect`] gives up and returns `Err`. +/// +/// Not the SDK's own default (`TcpClientReconnectionConfig::default()` is `max_retries: None` - +/// unlimited, one dial per second, forever). A Kafka client already retries at the wire-protocol +/// level once a handler maps a bridge failure to a retriable error code; the bridge blocking a +/// request task inside an unbounded internal reconnect loop would just add a second, invisible +/// retry layer underneath that one instead of surfacing the failure so the mapped code can be +/// sent. +/// +/// This bounds the *count*, not the *wall-clock time*, of that inner retry loop - see +/// [`CONNECT_TIMEOUT`] for the latter. +const RECONNECTION_RETRIES: u32 = 3; + +/// Wall-clock ceiling on the whole `client.connect()` call in [`IggyBridge::connect`], including +/// every attempt [`RECONNECTION_RETRIES`] makes internally. +/// +/// Without this, an unreachable-but-not-refusing address hangs far longer than "a few seconds": +/// `TcpClient::establish_bounded` only applies its own `FAILOVER_DIAL_TIMEOUT` (2s) when at least +/// two failover candidates are configured (`tcp_client.rs`) - a bridge always configures exactly +/// one address, so that guard never engages, and the plain `TcpStream::connect` underneath has no +/// deadline of its own. Against a firewall that drops SYN packets instead of refusing them, each +/// of the up to `RECONNECTION_RETRIES + 1` dial attempts pays the kernel's own SYN-retry timeout +/// (minutes, not seconds) rather than the `reconnection_interval` between attempts - a closed port +/// (instant RST) never exercises this path, so the failure mode only shows up in production. +/// 15s comfortably covers a slow-but-alive server's handshake (well above p99 login latency) while +/// still failing well short of the pathological multi-minute case. +const CONNECT_TIMEOUT: Duration = Duration::from_secs(15); + +/// Ceiling on a single Iggy client call made *after* [`IggyBridge::connect`] already succeeded - +/// `get_stream`, `create_stream`, `get_topic`, `create_topic`, and `shutdown`. +/// +/// [`CONNECT_TIMEOUT`] alone only bounds the *initial* connect. `TcpClient::send_raw_with_response` +/// (`tcp_client.rs`) reconnects internally on any transport error, mid-call, through the exact +/// same undead-lined `TcpStream::connect` path `CONNECT_TIMEOUT` exists to bound - so a bridge +/// call made after a successful `connect()` (a later network partition, Iggy moved to a new +/// address) could still hang on the kernel's own SYN-retry window with nothing here to stop it. +/// Same value as `CONNECT_TIMEOUT`: both bound the same underlying dial, just entered from a +/// different call site. +const REQUEST_TIMEOUT: Duration = CONNECT_TIMEOUT; + +/// Wraps a single Iggy client call in [`REQUEST_TIMEOUT`]. See that constant's doc for why every +/// bridge method needs this, not just [`IggyBridge::connect`]. +async fn with_request_timeout<T>( + op: impl Future<Output = Result<T, IggyError>>, +) -> Result<T, BridgeError> { + tokio::time::timeout(REQUEST_TIMEOUT, op) + .await + .map_err(|_elapsed| BridgeError::Iggy(IggyError::CannotEstablishConnection))? + .map_err(BridgeError::Iggy) +} + +/// Owns one connected `IggyClient` and resolves Kafka topics against it. +/// +/// Produce/Fetch handler wiring is a separate, later change (`#3535`/`#3536`) - this type is the +/// shared plumbing those handlers will call into, exercised standalone here via its own tests and +/// an integration test against a real `iggy-server`. +/// +/// One `IggyClient`, shared across every Kafka connection this gateway serves - and the SDK's TCP +/// transport is lockstep, one request in flight at a time (`tcp_client.rs`: "the connection is +/// lockstep", its stream mutex held across write, flush, and read). Every concurrent Kafka +/// connection ends up serialized behind whichever single Iggy request is in flight; the Kafka +/// side's own connection limit does nothing to relieve this. No pooling exists yet - `close` +/// already takes `self` by value, which anticipates an eventual `Arc`-shared bridge, but nothing +/// wires that up today. Documented here and in the README rather than silently discovered under +/// load once `#3535`/`#3536` land. +pub struct IggyBridge { + client: IggyClient, + config: IggyBridgeConfig, +} + +impl IggyBridge { + /// Connects to Iggy using `config` and authenticates. + /// + /// Builds the client through the SDK's fluent TCP builder rather than hand-assembling an + /// `iggy://user:pass@host` connection string: that string format splits on `@` then `:`, so a + /// password containing either character (`p@ss:word`) would be misparsed into a garbled + /// address instead of failing with a diagnosable config error. The fluent builder passes + /// `username`/`password` as already-separated fields, sidestepping the ambiguity entirely. + /// + /// # Errors + /// + /// Returns [`BridgeError::InvalidConfig`] if `config.address` is empty. Returns + /// [`BridgeError::Iggy`] if the address is malformed, the TCP connection fails, connecting + /// takes longer than `CONNECT_TIMEOUT` (an unreachable-and-silently-dropping address, not + /// just a refused one, is covered - see that constant's doc), or authentication is rejected - + /// this is the boundary [`BridgeError::to_kafka_error_code`] exists for: a handler calling + /// this must map the error to a wire response, never panic or unwrap, since an unreachable + /// Iggy backend is an expected runtime condition, not a bug. + pub async fn connect(config: IggyBridgeConfig) -> Result<Self, BridgeError> { + if config.address.trim().is_empty() { + return Err(BridgeError::InvalidConfig( + "Iggy address must not be empty".to_string(), + )); + } + + let credentials = + Credentials::UsernamePassword(config.username.clone(), config.password.clone()); + let client = IggyClientBuilder::new() + .with_tcp() + .with_server_address(config.address.clone()) + .with_auto_sign_in(AutoLogin::Enabled(credentials)) + .with_reconnection_max_retries(Some(RECONNECTION_RETRIES)) + .build() + .map_err(BridgeError::Iggy)?; + with_request_timeout(client.connect()).await?; Review Comment: Merged CONNECT_TIMEOUT/REQUEST_TIMEOUT into one constant. -- 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]
