Thanks Kirk, I'm overall +1 (binding). I think the timeouts may need a bit
of tweaking still. We can update the KIP and the thread if we decide to do
that as part of the PR review.

Ismael

On Thu, Oct 14, 2021 at 11:59 AM Kirk True <k...@mustardgrain.com> wrote:

> Hi Ismael,
>
> Thanks for reviewing the KIP. I've made a first pass at updating based on
> your feedback.
>
> Questions/comments inline...
>
> On Thu, Oct 14, 2021, at 6:20 AM, Ismael Juma wrote:
>
> Hi Kirk,
>
> Thanks for the KIP. It looks good overall to me. A few nits:
>
> 1. "sasl.login.retry.wait.ms": these configs are typically called
> `backoff`
> in Kafka. For example "retry.backoff.ms". The default for `
> retry.backoff.ms`
> is 100ms. Is there a reason why we are using a different value for this
> one? The `sasl.login.retry.max.wait.ms` should be renamed accordingly.
>
>
>
> Changed to sasl.login.retry.backoff.ms and sasl.login.retry.backoff.max.ms
> and changed the former to 100 ms.
>
> 2. "sasl.login.attempts": do we need this at all? We have generally moved
> away from number of retries in favor of timeouts for Kafka (the producer
> has a retries config partly for historical reasons, but partly due to
> semantics that are specific to the producer.
>
>
> Removed this option and now we just retry up to
> sasl.login.retry.backoff.max.ms.
>
> 3. "sasl.login.read.timeout.ms" : we have two types of kafka timeouts, "
> default.api.timeout.ms" and "request.timeout.ms". Is this similar to any
> of
> the two or is it different? If similar to one of the existing ones, we
> should name it similarly.
>
>
> This is specifically for the setReadTimeout on java.net.URLConnection when
> making the call to the OAuth/OIDC provider to retrieve the token. So it is
> SASL specific because reading the response from the external OAuth/OIDC
> provider (likely over WAN) may require a longer timeout.
>
> 4. "sasl.login.connect.timeout.ms": is this the equivalent of "
> socket.connection.setup.timeout.ms" in Kafka? I am unsure why we chose
> such
> a long name, "connect.timeout.ms" would have been a lot better. However,
> if
> it is similar, then we may want to follow the same naming convention.
>
>
> This is for the setConnectTimeout on java.net.URLConnection, similar to
> the above.
>
> 5. Should there be a "connect.max.timeout.ms" too?
>
>
> AFAIK, we don't have that level of control per our use of URLConnection.
>
> 6. What are the compatibility guarantees offered by the
> "OAuthCompatibilityTest" CLI tool? Also, can we adjust the name so it's
> clear that it's a Command versus a test suite?
>
>
> I changed the name to OAuthCompatibilityTool. Can you elaborate on what
> compatibility guarantees you'd like to see listed? I may just be
> misunderstanding the request.
>
> Thanks,
> Kirk
>
>
> Thanks!
>
> Ismael
>
> On Mon, Sep 27, 2021 at 10:20 AM Kirk True <k...@mustardgrain.com> wrote:
>
> > Hi all!
> >
> > I'd like to start a vote for KIP-768 that allows Kafka to connect to an
> > OAuth/OIDC identity provider for authentication and token retrieval:
> >
> >
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=186877575
> >
> > Thanks!
> > Kirk
>
>
>

Reply via email to