why the -1 and +1 after?

Ah this one got a little complicated because the first retry for a request can be set in two spots:

If the retryable grpc queue is empty/idle we set it here:

Hence I need to bump the attempt number before I push it back into the queue otherwise there's no way for me to tell whether this should be bumped to 1 or its the first retry and I should use 0. So since I bump it before I push it in the queue, the prior time is attempt_number - 1, the current one is attempt_number, and the next one is attempt_number + 1.

isn't this just accessed from within the class so we don't need getter/setter

This is accessed with the parent class RetryableGrpcClient, so we still need the getters/setters to access members within the nested class RetryableGrpcRequest.

wait the whole RetryableGrpcRequest class is private inside RetryableGrpcClient, i feel like there's no point to the encapsulation

But nvm that, I think attempt number should live on the client instead of on the request. I think we talked about this before but the "attempt #" we base the backoff on is technically the client-wide

this looks really hacky. i'm probably missing some context. why is the behavior different for GCS and why are we matching against a magic string to implement it?

Yea I agree this is hacky, but I think the behavior has to be different for the gcs vs the raylet/core worker client pool.
The motivation behind this PR is to remove all the individual timeouts for the raylet/core worker client pool. I had to reduce the timeout to sub 1s for a couple of tests, hence I had the raylet/core worker client pool server timeouts be initially 100ms. After talking to @dayshah we decided it would be cleaner to put it as 1s, but for the first retry trigger the default_unavailable_timeout_callback immediately to detect node death ASAP and stop retries. I can't trigger this for the gcs default unavailable timeout callback though, because it doesn't have any retries and just kills the process without waiting for gcs to potentially restart:

ok I get the motivation now. instead of hard coding a match against the server name string, let's make this an explicit param to the retryable client that is passed by all of the components, like: first_server_unavailable_timeout=0 or call_first_unavailable_callback_immediately=true

+1. We can just pass in exponential backoff configs explicitly: initial delay, exponential factor, max delay.

Sounds good, added:
call_first_unavailable_timeout_callback_immediately_
exponential_factor_
max_backoff_seconds_
as explicit parameters to the RetryableGrpcClient. Initial delay should be covered by the *_server_reconnect_base_timeout_s already

this should be capped, else the timeout will approach infinity quickly

gotcha, I'll add a cap of 60 seconds

I think we just need three configs:

server_unavailable_timeout_initial_seconds, server_unavailable_timeout_max_seconds, and server_unavailable_timeout_exponential_factor?

How I thought of call_first_unavailable_timeout_callback_immediately to be is to add an extra iteration of 0 in front of the sequence, so we'll start off at:
0 and trigger the channel check-> server_unavailable_base_timeout_seconds -> 2 * server_unavailable_base_timeout_seconds...

Otherwise it it's false then we have the normal sequence of:
server_unavailable_base_timeout_seconds -> 2 * server_unavailable_base_timeout_seconds -> ...

If we just set server_unavailable_base_timeout_seconds to 0 then its a question of what is getting multiplied by the exponential factor. I could hardcode it to 1s, but think that would be confusing.

server_unavailable_timeout_exponential_factor is always going to be the same, if it doesn't need to be a param don't make it a param imo, saves an extra hop when reading the code. And we're adding an extra default param that's never set to GetBackoffMilliseconds now

Yea good point... I ended end just keep server_unavailable_max_timeout_seconds since
server_unavailable_timeout_initial_seconds is not needed after: #56613

nit but can you make reset_timer not true as a default parameter, imo easier to read if you don't have to lookup the not obvious default param

Yea good point, I ended up just removing the default parameter since its literally only used in one place

wait the whole RetryableGrpcRequest class is private inside RetryableGrpcClient, i feel like there's no point to the encapsulation

But nvm that, I think attempt number should live on the client instead of on the request. I think we talked about this before but the "attempt #" we base the backoff on is technically the client-wide

server_unavailable_timeout_exponential_factor is always going to be the same, if it doesn't need to be a param don't make it a param imo, saves an extra hop when reading the code. And we're adding an extra default param that's never set to GetBackoffMilliseconds now

Bug: GCS Client Retry Timeout Configuration Issue

The GCS client's RetryableGrpcClient is configured with identical values for client_reconnect_timeout_base_seconds and client_reconnect_timeout_max_seconds. This disables exponential backoff, causing retries to use a fixed timeout instead of an exponentially increasing one, unlike other clients like CoreWorker and Raylet.

Check to see if user depends on this config

(this is why we need proper Ray config w/ stabilities)

Bug: GCS Client Retry Timeout Configuration Issue

The GCS client's RetryableGrpcClient sets both client_reconnect_timeout_base_seconds and client_reconnect_timeout_max_seconds to the same config value. This prevents exponential backoff from working, resulting in constant retry intervals. This behavior is inconsistent with other clients like raylet and core worker, which use separate base and max timeouts.

Bug: GCS Client Retry Timeout Configuration Issue

The GCS client is configured with the same value for both server_reconnect_timeout_base_seconds and server_reconnect_timeout_max_seconds (gcs_rpc_server_reconnect_timeout_s()). This effectively disables exponential backoff, causing retry intervals to remain constant (e.g., 60 seconds) instead of increasing, which is inconsistent with how core worker and raylet clients are set up.

Bug: GCS Client Configuration Prevents Exponential Backoff

The GCS client passes the same gcs_rpc_server_reconnect_timeout_s config value to both server_reconnect_timeout_base_seconds and server_reconnect_timeout_max_seconds. This prevents exponential backoff from functioning, causing reconnection attempts to use a constant timeout instead of an increasing one.