fix: Only connect lazily (remove option to connect eagerly)

[vrongmeal]

Resolves: #43

[shikhar]

Do you see a way to avoid the bool for force_lazy_connection?

[shikhar]

please also revisit the internal bools around this

[shikhar]

I think I'd prefer if we just went with lazy, always.

tonic::Channel will internally attempt to reconnect, so the possibility exists already of a connection error happening at request time.

Separately, let's consider ways in which we can clearly distinguish requests that were never transmitted due to failing at the connection stage, since those can be safely retried even if non-idempotent.

[vrongmeal]

Separately, let's consider ways in which we can clearly distinguish requests that were never transmitted due to failing at the connection stage, since those can be safely retried even if non-idempotent.

As a follow-up will take on looking at this.

[vrongmeal]

@infiniteregrets something to note here in this PR, the connect methods have been made sync now since we're connecting lazily.

Moreover, maybe we should name them new instead of connect now?

[shikhar]

yes, let's rename to new

[shikhar]

Re: #54 - I guess those methods should be called new_with_connector?

[vrongmeal]

WRT #54, ClientInner::connect_with_connector uses some different configurations for connecting with the endpoint, namely:

            .keep_alive_timeout(Duration::from_secs(5))
            .http2_keep_alive_interval(Duration::from_secs(5))

And it does not use a TLS config, which is used in the original connection:

            .tls_config(
                ClientTlsConfig::default()
                    .with_webpki_roots()
                    .assume_http2(true),
            )?

Are these supposed to be different? I was thinking of combining the functions. Maybe we have a flag for TLS configuration and if required, configs for keep alive timeouts as well?

[shikhar]

The connector feature is intended for only internal consumption for now (from turmoil DST).

The key difference is we are using http as the scheme - I guess we could put that on ClientConfig? It may be useful when we have an emulator. Setting .tls_config() should be fine in this case too as it is only exercised for https.

Re: keep_alive_timeout(Duration::from_secs(5)) & http2_keep_alive_interval(Duration::from_secs(5)), I think that must have crept in inadvertently. Let's remove for now till we can properly consider what default knobs are appropriate for http2 & tcp keepalive from a client to s2 endpoint.

[infiniteregrets]

keep_alive_timeout(Duration::from_secs(5)) & http2_keep_alive_interval(Duration::from_secs(5))

I think I copied that directly from the endpoint config from the DST

[vrongmeal]

@infiniteregrets can you try this branch out once for your DST stuff?

The connector stuff is still gated behind the feature but I have made the dependencies non-optional since they are already hard dependencies on tonic (at least for us). This makes the code a bit cleaner.

[shikhar]

I think better to panic in this case

[shikhar]

LG aside from avoiding a new error variant, any integration issue for the connector feature can be a followup