Skip to content

Commit

Permalink
Remove client_creation_timeout configuration
Browse files Browse the repository at this point in the history
  • Loading branch information
barshaul committed Nov 16, 2023
1 parent b3c7e9c commit a93d999
Show file tree
Hide file tree
Showing 10 changed files with 22 additions and 95 deletions.
12 changes: 3 additions & 9 deletions babushka-core/src/client/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -237,8 +237,6 @@ fn sanitized_request_string(request: &ConnectionRequest) -> String {
let tls_mode = request.tls_mode.enum_value_or_default();
let cluster_mode = request.cluster_mode_enabled;
let request_timeout = format_non_zero_value("response timeout", request.request_timeout);
let client_creation_timeout =
format_non_zero_value("client creation timeout", request.client_creation_timeout);
let database_id = format_non_zero_value("database ID", request.database_id);
let rfr_strategy = request.read_from.enum_value_or_default();
let connection_retry_strategy = match &request.connection_retry_strategy.0 {
Expand All @@ -250,24 +248,20 @@ fn sanitized_request_string(request: &ConnectionRequest) -> String {
};

format!(
"\naddresses: {addresses}\nTLS mode: {tls_mode:?}\ncluster mode: {cluster_mode}{request_timeout}{client_creation_timeout}\nRead from replica strategy: {rfr_strategy:?}{database_id}{connection_retry_strategy}",
"\naddresses: {addresses}\nTLS mode: {tls_mode:?}\ncluster mode: {cluster_mode}{request_timeout}\nRead from replica strategy: {rfr_strategy:?}{database_id}{connection_retry_strategy}",
)
}

impl Client {
pub async fn new(request: ConnectionRequest) -> Result<Self, ConnectionError> {
const DEFAULT_CLIENT_CREATION_TIMEOUT: Duration = Duration::from_millis(2500);
const DEFAULT_CLIENT_CREATION_TIMEOUT: Duration = Duration::from_secs(10);

log_info(
"Connection configuration",
sanitized_request_string(&request),
);
let request_timeout = to_duration(request.request_timeout, DEFAULT_RESPONSE_TIMEOUT);
let total_connection_timeout = to_duration(
request.client_creation_timeout,
DEFAULT_CLIENT_CREATION_TIMEOUT,
);
tokio::time::timeout(total_connection_timeout, async move {
tokio::time::timeout(DEFAULT_CLIENT_CREATION_TIMEOUT, async move {
let internal_client = if request.cluster_mode_enabled {
let client = create_cluster_client(request)
.await
Expand Down
9 changes: 4 additions & 5 deletions babushka-core/src/protobuf/connection_request.proto
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,10 @@ message ConnectionRequest {
TlsMode tls_mode = 2;
bool cluster_mode_enabled = 3;
uint32 request_timeout = 4;
uint32 client_creation_timeout = 5;
ReadFrom read_from = 6;
ConnectionRetryStrategy connection_retry_strategy = 7;
AuthenticationInfo authentication_info = 8;
uint32 database_id = 9;
ReadFrom read_from = 5;
ConnectionRetryStrategy connection_retry_strategy = 6;
AuthenticationInfo authentication_info = 7;
uint32 database_id = 8;
}

message ConnectionRetryStrategy {
Expand Down
45 changes: 0 additions & 45 deletions babushka-core/tests/test_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,49 +190,4 @@ mod shared_client_tests {
assert!(err.is_timeout(), "{err}");
});
}

#[rstest]
#[timeout(SHORT_CLUSTER_TEST_TIMEOUT)]
fn test_connection_timeout(#[values(false, true)] use_cluster: bool) {
let use_tls = true;
async fn expect_timeout_on_client_creation(
addresses: &[redis::ConnectionAddr],
cluster_mode: ClusterMode,
use_tls: bool,
) {
let mut configuration = TestConfiguration {
cluster_mode,
use_tls,
..Default::default()
};

configuration.connection_timeout = Some(1);
let err = Client::new(create_connection_request(addresses, &configuration))
.await
.map(|_| ())
.unwrap_err();
assert!(matches!(err, babushka::client::ConnectionError::Timeout));
}

block_on_all(async {
if use_cluster {
let cluster = RedisCluster::new(use_tls, &None, Some(3), Some(0));
expect_timeout_on_client_creation(
&cluster.get_server_addresses(),
ClusterMode::Enabled,
use_tls,
)
.await;
} else {
let server = RedisServer::new(ServerType::Tcp { tls: use_tls });
wait_for_server_to_become_ready(&server.get_client_addr()).await;
expect_timeout_on_client_creation(
&[server.get_client_addr()],
ClusterMode::Disabled,
use_tls,
)
.await;
}
});
}
}
1 change: 0 additions & 1 deletion babushka-core/tests/utilities/cluster.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,6 @@ pub async fn setup_test_basics_internal(mut configuration: TestConfiguration) ->
}
configuration.cluster_mode = ClusterMode::Enabled;
configuration.request_timeout = configuration.request_timeout.or(Some(10000));
configuration.connection_timeout = configuration.connection_timeout.or(Some(10000));
let connection_request = create_connection_request(&addresses, &configuration);

let client = Client::new(connection_request).await.unwrap();
Expand Down
4 changes: 0 additions & 4 deletions babushka-core/tests/utilities/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -566,9 +566,6 @@ pub fn create_connection_request(
if let Some(request_timeout) = configuration.request_timeout {
connection_request.request_timeout = request_timeout;
}
if let Some(connection_timeout) = configuration.connection_timeout {
connection_request.client_creation_timeout = connection_timeout;
}
if let Some(strategy) = configuration.read_from {
connection_request.read_from = strategy.into()
}
Expand All @@ -589,7 +586,6 @@ pub struct TestConfiguration {
pub connection_info: Option<RedisConnectionInfo>,
pub cluster_mode: ClusterMode,
pub request_timeout: Option<u32>,
pub connection_timeout: Option<u32>,
pub shared_server: bool,
pub read_from: Option<connection_request::ReadFrom>,
pub database_id: u32,
Expand Down
9 changes: 0 additions & 9 deletions node/src/BaseClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,14 +120,6 @@ export type BaseClientConfiguration = {
* Value must be an integer.
*/
requestTimeout?: number;
/**
* The duration in milliseconds that the client should wait its initialization,
* encompassing tasks such as connection establishment and topology mapping.
* If the client fails to complete its initialization within this specified time, an error will be returned.
* If not set, a default value will be used.
* Value must be an integer.
*/
clientCreationTimeout?: number;
/**
* Represents the client's read from strategy.
* If not set, `Primary` will be used.
Expand Down Expand Up @@ -876,7 +868,6 @@ export class BaseClient {
: connection_request.TlsMode.NoTls,
requestTimeout: options.requestTimeout,
clusterModeEnabled: false,
clientCreationTimeout: options.clientCreationTimeout,
readFrom,
authenticationInfo,
};
Expand Down
20 changes: 0 additions & 20 deletions python/python/pybushka/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,6 @@ def __init__(
use_tls: bool = False,
credentials: Optional[RedisCredentials] = None,
read_from: ReadFrom = ReadFrom.PRIMARY,
client_creation_timeout: Optional[int] = None,
request_timeout: Optional[int] = None,
):
"""
Expand All @@ -103,10 +102,6 @@ def __init__(
credentials (RedisCredentials): Credentials for authentication process.
If none are set, the client will not authenticate itself with the server.
read_from (ReadFrom): If not set, `PRIMARY` will be used.
client_creation_timeout (Optional[int]): The duration in milliseconds that the client should wait its initialization,
encompassing tasks such as connection establishment and topology mapping.
If the client fails to complete its initialization within this specified time, an error will be returned.
If not set, a default value will be used.
request_timeout (Optional[int]): The duration in milliseconds that the client should wait for a request to complete.
This duration encompasses sending the request, awaiting for a response from the server, and any required reconnections or retries.
If the specified timeout is exceeded for a pending request, it will result in a timeout error. If not set, a default value will be used.
Expand All @@ -115,7 +110,6 @@ def __init__(
self.use_tls = use_tls
self.credentials = credentials
self.read_from = read_from
self.client_creation_timeout = client_creation_timeout
self.request_timeout = request_timeout

def _create_a_protobuf_conn_request(
Expand All @@ -139,8 +133,6 @@ def _create_a_protobuf_conn_request(
request.read_from = self.read_from.value
if self.request_timeout:
request.request_timeout = self.request_timeout
if self.client_creation_timeout:
request.client_creation_timeout = self.client_creation_timeout
request.cluster_mode_enabled = True if cluster_mode else False
if self.credentials:
if self.credentials.username:
Expand All @@ -167,10 +159,6 @@ class RedisClientConfiguration(BaseClientConfiguration):
credentials (RedisCredentials): Credentials for authentication process.
If none are set, the client will not authenticate itself with the server.
read_from (ReadFrom): If not set, `PRIMARY` will be used.
client_creation_timeout (Optional[int]): The duration in milliseconds that the client should wait its initialization,
encompassing tasks such as connection establishment and topology mapping.
If the client fails to complete its initialization within this specified time, an error will be returned.
If not set, a default value will be used.
request_timeout (Optional[int]): The duration in milliseconds that the client should wait for a request to complete.
This duration encompasses sending the request, awaiting for a response from the server, and any required reconnections or retries.
If the specified timeout is exceeded for a pending request, it will result in a timeout error.
Expand All @@ -187,7 +175,6 @@ def __init__(
use_tls: bool = False,
credentials: Optional[RedisCredentials] = None,
read_from: ReadFrom = ReadFrom.PRIMARY,
client_creation_timeout: Optional[int] = None,
request_timeout: Optional[int] = None,
reconnect_strategy: Optional[BackoffStrategy] = None,
database_id: Optional[int] = None,
Expand All @@ -197,7 +184,6 @@ def __init__(
use_tls=use_tls,
credentials=credentials,
read_from=read_from,
client_creation_timeout=client_creation_timeout,
request_timeout=request_timeout,
)
self.reconnect_strategy = reconnect_strategy
Expand Down Expand Up @@ -238,10 +224,6 @@ class ClusterClientConfiguration(BaseClientConfiguration):
credentials (RedisCredentials): Credentials for authentication process.
If none are set, the client will not authenticate itself with the server.
read_from (ReadFrom): If not set, `PRIMARY` will be used.
client_creation_timeout (Optional[int]): The duration in milliseconds that the client should wait its initialization,
encompassing tasks such as connection establishment and topology mapping.
If the client fails to complete its initialization within this specified time, an error will be returned.
If not set, a default value will be used.
request_timeout (Optional[int]): The duration in milliseconds that the client should wait for a request to complete.
This duration encompasses sending the request, awaiting for a response from the server, and any required reconnections or retries.
If the specified timeout is exceeded for a pending request, it will result in a timeout error. If not set, a default value will be used.
Expand All @@ -257,14 +239,12 @@ def __init__(
use_tls: bool = False,
credentials: Optional[RedisCredentials] = None,
read_from: ReadFrom = ReadFrom.PRIMARY,
client_creation_timeout: Optional[int] = None,
request_timeout: Optional[int] = None,
):
super().__init__(
addresses=addresses,
use_tls=use_tls,
credentials=credentials,
read_from=read_from,
client_creation_timeout=client_creation_timeout,
request_timeout=request_timeout,
)
2 changes: 2 additions & 0 deletions python/python/pybushka/pybushka.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ from typing import Optional

from pybushka.constants import TResult

DEFAULT_TIMEOUT_IN_MILLISECONDS: int = ...

class Level(Enum):
Error = 0
Warn = 1
Expand Down
8 changes: 6 additions & 2 deletions python/python/pybushka/redis_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,11 @@
from pybushka.routes import Route, set_protobuf_route
from typing_extensions import Self

from .pybushka import start_socket_listener_external, value_from_pointer
from .pybushka import (
DEFAULT_TIMEOUT_IN_MILLISECONDS,
start_socket_listener_external,
value_from_pointer,
)


def get_request_error_class(
Expand Down Expand Up @@ -100,7 +104,7 @@ def init_callback(socket_path: Optional[str], err: Optional[str]):
async def _create_uds_connection(self) -> None:
try:
# Open an UDS connection
async with async_timeout.timeout(self.config.client_creation_timeout):
async with async_timeout.timeout(DEFAULT_TIMEOUT_IN_MILLISECONDS):
reader, writer = await asyncio.open_unix_connection(
path=self.socket_path
)
Expand Down
7 changes: 7 additions & 0 deletions python/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ use pyo3::Python;

use redis::Value;

pub const DEFAULT_TIMEOUT_IN_MILLISECONDS: u32 =
babushka::client::DEFAULT_RESPONSE_TIMEOUT.as_millis() as u32;

#[pyclass]
#[derive(PartialEq, Eq, PartialOrd, Clone)]
pub enum Level {
Expand All @@ -28,6 +31,10 @@ impl Level {
#[pymodule]
fn pybushka(_py: Python, m: &PyModule) -> PyResult<()> {
m.add_class::<Level>()?;
m.add(
"DEFAULT_TIMEOUT_IN_MILLISECONDS",
DEFAULT_TIMEOUT_IN_MILLISECONDS,
)?;

#[pyfn(m)]
fn py_log(log_level: Level, log_identifier: String, message: String) {
Expand Down

0 comments on commit a93d999

Please sign in to comment.