Hi Andrew and all, I did the mentioned change.
As there are no more comments, I'm letting this sit for e.g. a week and then I will call for the vote. Best, Ivan On Tue, Apr 9, 2024, at 23:51, Andrew Schofield wrote: > Hi Ivan, > I think you have to go one way or the other with the cluster ID, so I think > removing that from this KIP might > be the best. I think there’s another KIP waiting to be written for ensuring > consistency of clusters, but > I think that wouldn’t conflict at all with this one. > > Thanks, > Andrew > > > On 9 Apr 2024, at 19:11, Ivan Yurchenko <i...@ivanyu.me> wrote: > > > > Hi Andrew and all, > > > > I looked deeper into the code [1] and it seems the Metadata class is OK > > with cluster ID changing. So I'm thinking that the rebootstrapping > > shouldn't introduce a new failure mode here. And I should remove the > > mention of this cluster ID checks from the KIP. > > > > Best, > > Ivan > > > > [1] > > https://github.com/apache/kafka/blob/ff90f78c700c582f9800013faad827c36b45ceb7/clients/src/main/java/org/apache/kafka/clients/Metadata.java#L355 > > > > On Tue, Apr 9, 2024, at 09:28, Andrew Schofield wrote: > >> Hi Ivan, > >> Thanks for the KIP. I can see situations in which this would be helpful. I > >> have one question. > >> > >> The KIP says the client checks the cluster ID when it re-bootstraps and > >> that it will fail if the > >> cluster ID doesn’t match the previously known one. How does it fail? Which > >> exception does > >> it throw and when? > >> > >> In a similar vein, now that we are checking cluster IDs, I wonder if it > >> could be extended to > >> cover all situations in which there are cluster ID mismatches, such as the > >> bootstrap server > >> list erroneously pointing at brokers from different clusters and the > >> problem only being > >> detectable later on. > >> > >> Thanks, > >> Andrew > >> > >>> On 8 Apr 2024, at 18:24, Ivan Yurchenko <i...@ivanyu.me> wrote: > >>> > >>> Hello! > >>> > >>> I changed the KIP a bit, specifying that the certain benefit goes to > >>> consumers not participating in a group, but that other clients can > >>> benefit as well in certain situations. > >>> > >>> You can see the changes in the history [1] > >>> > >>> Thank you! > >>> > >>> Ivan > >>> > >>> [1] > >>> https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=240881396&originalVersion=10&revisedVersion=11 > >>> > >>> On 2023/07/15 16:37:52 Ivan Yurchenko wrote: > >>>> Hello! > >>>> > >>>> I've made several changes to the KIP based on the comments: > >>>> > >>>> 1. Reduced the scope to producer and consumer clients only. > >>>> 2. Added more details to the description of the rebootstrap process. > >>>> 3. Documented the role of low values of reconnect.backoff.max.ms in > >>>> preventing rebootstrapping. > >>>> 4. Some wording changes. > >>>> > >>>> You can see the changes in the history [1] > >>>> > >>>> I'm planning to put the KIP to a vote in some days if there are no new > >>>> comments. > >>>> > >>>> Thank you! > >>>> > >>>> Ivan > >>>> > >>>> [1] > >>>> https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=240881396&selectedPageVersions=9&selectedPageVersions=5 > >>>> > >>>> On Tue, 30 May 2023 at 08:23, Ivan Yurchenko <iv...@gmail.com> > >>>> wrote: > >>>> > >>>>> Hi Chris and all, > >>>>> > >>>>>> I believe the logic you've linked is only applicable for the producer > >>>>>> and > >>>>>> consumer clients; the admin client does something different (see [1]). > >>>>> > >>>>> I see, thank you for the pointer. It seems the admin client is fairly > >>>>> different from the producer and consumer. Probably it makes sense to > >>>>> reduce > >>>>> the scope of the KIP to the producer and consumer clients only. > >>>>> > >>>>>> it'd be nice to have a definition of when re-bootstrapping > >>>>>> would occur that doesn't rely on internal implementation details. What > >>>>>> user-visible phenomena can we identify that would lead to a > >>>>>> re-bootstrapping? > >>>>> > >>>>> Let's put it this way: "Re-bootstrapping means that the client forgets > >>>>> about nodes it knows about and falls back on the bootstrap nodes as if > >>>>> it > >>>>> had just been initialized. Re-bootstrapping happens when, during a > >>>>> metadata > >>>>> update (which may be scheduled by `metadata.max.age.ms` or caused by > >>>>> certain error responses like NOT_LEADER_OR_FOLLOWER, > >>>>> REPLICA_NOT_AVAILABLE, > >>>>> etc.), the client doesn't have a node with an established connection or > >>>>> establishable connection." > >>>>> Does this sound good? > >>>>> > >>>>>> I also believe that if someone has " > >>>>>> reconnect.backoff.max.ms" set to a low-enough value, > >>>>>> NetworkClient::leastLoadedNode may never return null. In that case, > >>>>>> shouldn't we still attempt a re-bootstrap at some point (if the user > >>>>>> has > >>>>>> enabled this feature)? > >>>>> > >>>>> Yes, you're right. Particularly `canConnect` here [1] can always be > >>>>> returning `true` if `reconnect.backoff.max.ms` is low enough. > >>>>> It seems pretty difficult to find a good criteria when re-bootstrapping > >>>>> should be forced in this case, so it'd be difficult to configure and > >>>>> reason > >>>>> about. I think it's worth mentioning in the KIP and later in the > >>>>> documentation, but we should not try to do anything special here. > >>>>> > >>>>>> Would it make sense to re-bootstrap only after " > >>>>>> metadata.max.age.ms" has elapsed since the last metadata update, and > >>>>> when > >>>>>> at least one request has been made to contact each known server and > >>>>>> been > >>>>>> met with failure? > >>>>> > >>>>> The first condition is satisfied by the check in the beginning of > >>>>> `maybeUpdate` [2]. > >>>>> It seems to me, the second one is also satisfied by `leastLoadedNode`. > >>>>> Admittedly, it's more relaxed than you propose: it tracks > >>>>> unavailability of > >>>>> nodes that was detected by all types of requests, not only by metadata > >>>>> requests. > >>>>> What do you think, would this be enough? > >>>>> > >>>>> [1] > >>>>> https://github.com/apache/kafka/blob/c9a42c85e2c903329b3550181d230527e90e3646/clients/src/main/java/org/apache/kafka/clients/NetworkClient.java#L698 > >>>>> [2] > >>>>> https://github.com/apache/kafka/blob/c9a42c85e2c903329b3550181d230527e90e3646/clients/src/main/java/org/apache/kafka/clients/NetworkClient.java#L1034-L1041 > >>>>> > >>>>> Best, > >>>>> Ivan > >>>>> > >>>>> > >>>>> On Tue, 21 Feb 2023 at 20:07, Chris Egerton <ch...@aiven.io.invalid> > >>>>> wrote: > >>>>> > >>>>>> Hi Ivan, > >>>>>> > >>>>>> I believe the logic you've linked is only applicable for the producer > >>>>>> and > >>>>>> consumer clients; the admin client does something different (see [1]). > >>>>>> > >>>>>> Either way, it'd be nice to have a definition of when re-bootstrapping > >>>>>> would occur that doesn't rely on internal implementation details. What > >>>>>> user-visible phenomena can we identify that would lead to a > >>>>>> re-bootstrapping? I also believe that if someone has " > >>>>>> reconnect.backoff.max.ms" set to a low-enough value, > >>>>>> NetworkClient::leastLoadedNode may never return null. In that case, > >>>>>> shouldn't we still attempt a re-bootstrap at some point (if the user > >>>>>> has > >>>>>> enabled this feature)? Would it make sense to re-bootstrap only after " > >>>>>> metadata.max.age.ms" has elapsed since the last metadata update, and > >>>>>> when > >>>>>> at least one request has been made to contact each known server and > >>>>>> been > >>>>>> met with failure? > >>>>>> > >>>>>> [1] - > >>>>>> > >>>>>> https://github.com/apache/kafka/blob/c9a42c85e2c903329b3550181d230527e90e3646/clients/src/main/java/org/apache/kafka/clients/admin/internals/AdminMetadataManager.java#L100 > >>>>>> > >>>>>> Cheers, > >>>>>> > >>>>>> Chris > >>>>>> > >>>>>> On Sun, Feb 19, 2023 at 3:39 PM Ivan Yurchenko <iv...@gmail.com> > >>>>>> wrote: > >>>>>> > >>>>>>> Hi Chris, > >>>>>>> > >>>>>>> Thank you for your question. As a part of various lifecycle phases > >>>>>>> (including node disconnect), NetworkClient can request metadata update > >>>>>>> eagerly (the `Metadata.requestUpdate` method), which results in > >>>>>>> `MetadataUpdater.maybeUpdate` being called during next poll. Inside, > >>>>>>> it > >>>>>> has > >>>>>>> a way to find a known node it can connect to for the fresh metadata. > >>>>>>> If > >>>>>> no > >>>>>>> such node is found, it backs off. (Code [1]). I'm thinking of > >>>>>> piggybacking > >>>>>>> on this logic and injecting the rebootstrap attempt before the > >>>>>>> backoff. > >>>>>>> > >>>>>>> As of the second part of you question: the re-bootstrapping means > >>>>>> replacing > >>>>>>> the node addresses in the client with the original bootstrap > >>>>>>> addresses, > >>>>>> so > >>>>>>> if the first bootstrap attempt fails, the client will continue using > >>>>>>> the > >>>>>>> bootstrap addresses until success -- pretty much as if it were > >>>>>>> recreated > >>>>>>> from scratch. > >>>>>>> > >>>>>>> Best, > >>>>>>> Ivan > >>>>>>> > >>>>>>> [1] > >>>>>>> > >>>>>>> > >>>>>> https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/clients/NetworkClient.java#L1045-L1049 > >>>>>>> > >>>>>>> On Thu, 16 Feb 2023 at 17:18, Chris Egerton <ch...@aiven.io.invalid> > >>>>>>> wrote: > >>>>>>> > >>>>>>>> Hi Ivan, > >>>>>>>> > >>>>>>>> I'm not very familiar with the clients side of things but the > >>>>>>>> proposal > >>>>>>>> seems reasonable. > >>>>>>>> > >>>>>>>> I like the flexibility of the "metadata.recovery.strategy" property > >>>>>> as a > >>>>>>>> string instead of, e.g., a "rebootstrap.enabled" boolean. We may want > >>>>>> to > >>>>>>>> adapt a different approach in the future, like the background thread > >>>>>>>> mentioned in the rejected alternatives section. > >>>>>>>> > >>>>>>>> I also like handling this via configuration property instead of > >>>>>> adding a > >>>>>>>> Java-level API or suggesting that users re-instantiate their clients > >>>>>>> since > >>>>>>>> we may want to enable this new behavior by default in the future, and > >>>>>> it > >>>>>>>> also reduces the level of effort required for users to benefit from > >>>>>> this > >>>>>>>> improvement. > >>>>>>>> > >>>>>>>> One question I have--that may have an obvious answer to anyone more > >>>>>>>> familiar with client internals--is under which conditions we will > >>>>>>> determine > >>>>>>>> a rebootstrap is appropriate. Taking the admin client as an example, > >>>>>> the > >>>>>>> " > >>>>>>>> default.api.timeout.ms" property gives us a limit on the time an > >>>>>>> operation > >>>>>>>> will be allowed to take before it completes or fails (with optional > >>>>>>>> per-request overrides in the various *Options classes), and the " > >>>>>>>> request.timeout.ms" property gives us a limit on the time each > >>>>>> request > >>>>>>>> issued for that operation will be allowed to take before it > >>>>>> completes, is > >>>>>>>> retried, or causes the operation to fail (if no more retries can be > >>>>>>>> performed). If all of the known servers (i.e., bootstrap servers for > >>>>>> the > >>>>>>>> first operation, or discovered brokers if bootstrapping has already > >>>>>> been > >>>>>>>> completed) are unavailable, the admin client will keep (re)trying to > >>>>>>> fetch > >>>>>>>> metadata until the API timeout is exhausted, issuing multiple > >>>>>> requests to > >>>>>>>> the same server if necessary. When would a re-bootstrapping occur > >>>>>> here? > >>>>>>>> Ideally we could find some approach that minimizes false positives > >>>>>>> (where a > >>>>>>>> re-bootstrapping is performed even though the current set of known > >>>>>>> brokers > >>>>>>>> is only temporarily unavailable, as opposed to permanently moved). Of > >>>>>>>> course, given the opt-in nature of the re-bootstrapping feature, we > >>>>>> can > >>>>>>>> always shoot for "good enough" on that front, but, it'd be nice to > >>>>>>>> understand some of the potential pitfalls of enabling it. > >>>>>>>> > >>>>>>>> Following up on the above, would we cache the need to perform a > >>>>>>>> re-bootstrap across separate operations? For example, if I try to > >>>>>>> describe > >>>>>>>> a cluster, then a re-bootstrapping takes place and fails, and then I > >>>>>> try > >>>>>>> to > >>>>>>>> describe the cluster a second time. With that second attempt, would > >>>>>>>> we > >>>>>>>> immediately resort to the bootstrap servers for any initial metadata > >>>>>>>> updates, or would we still try to go through the last-known set of > >>>>>>> brokers > >>>>>>>> first? > >>>>>>>> > >>>>>>>> Cheers, > >>>>>>>> > >>>>>>>> Chris > >>>>>>>> > >>>>>>>> On Mon, Feb 6, 2023 at 4:32 AM Ivan Yurchenko < > >>>>>> ivan0yurche...@gmail.com> > >>>>>>>> wrote: > >>>>>>>> > >>>>>>>>> Hi! > >>>>>>>>> > >>>>>>>>> There seems to be not much more discussion going, so I'm planning to > >>>>>>>> start > >>>>>>>>> the vote in a couple of days. > >>>>>>>>> > >>>>>>>>> Thanks, > >>>>>>>>> > >>>>>>>>> Ivan > >>>>>>>>> > >>>>>>>>> On Wed, 18 Jan 2023 at 12:06, Ivan Yurchenko < > >>>>>> ivan0yurche...@gmail.com > >>>>>>>> > >>>>>>>>> wrote: > >>>>>>>>> > >>>>>>>>>> Hello! > >>>>>>>>>> I would like to start the discussion thread on KIP-899: Allow > >>>>>> clients > >>>>>>>> to > >>>>>>>>>> rebootstrap. > >>>>>>>>>> This KIP proposes to allow Kafka clients to repeat the bootstrap > >>>>>>>> process > >>>>>>>>>> when fetching metadata if none of the known nodes are available. > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-899%3A+Allow+clients+to+rebootstrap > >>>>>>>>>> > >>>>>>>>>> A question right away: should we eventually change the default > >>>>>>> behavior > >>>>>>>>> or > >>>>>>>>>> it can remain configurable "forever"? The latter is proposed in > >>>>>> the > >>>>>>>> KIP. > >>>>>>>>>> > >>>>>>>>>> Thank you! > >>>>>>>>>> > >>>>>>>>>> Ivan > > >
