Thanks Kamal for the KIP. This is an important enhancement to unblock the io threads as ListOffsetsAPI may need to fetch data from remote storage and it may take longer to process them. This feature helped in not starving other broker api requests in our environments when there are many ListOffsetsAPi requests on a broker.
Thanks, Satish. On Sat, 17 Aug 2024 at 13:55, Kamal Chandraprakash <kamal.chandraprak...@gmail.com> wrote: > > Hi all, > > If there are no more comments, I'll start a vote soon. > > Thanks, > Kamal > > On Wed, Aug 14, 2024 at 5:05 PM Luke Chen <show...@gmail.com> wrote: > > > Hi Kamal, > > > > Thanks for the update. > > LGTM. > > > > Luke > > > > On Wed, Aug 14, 2024 at 7:25 PM Kamal Chandraprakash < > > kamal.chandraprak...@gmail.com> wrote: > > > > > Hi all, > > > > > > > I saw we added some new configs/metrics. > > > > > > I have removed the recent changes to the public interfaces to limit the > > > scope of the KIP to minimum. PTAL. > > > > > > Thanks, > > > Kamal > > > > > > On Wed, Aug 14, 2024 at 9:58 AM Kamal Chandraprakash < > > > kamal.chandraprak...@gmail.com> wrote: > > > > > > > Hi Luke, > > > > > > > > > LC5 > > > > Agree, this is a rare scenario. Given that we have a common pool of > > > > request handler threads to accept > > > > all the incoming requests and there are no quotas to handle for each > > > > request. I'm OK with reusing the > > > > same remote-log-reader threads for LIST_OFFSETS requests. There may be > > > > noisy neighbor issues > > > > in handling the LIST_OFFSETS and FETCH remote requests when we read > > from > > > > remote storage > > > > aggressively and all the remote-log-reader threads are busy. > > > > > > > > > If so, maybe the additional config/metrics are not necessary? > > > > Do you mean to have a separate thread pool and hardcode the num > > threads? > > > > > > > > > LC6 and LC7 > > > > Updated the KIP. > > > > > > > > Thanks, > > > > Kamal > > > > > > > > On Wed, Aug 14, 2024 at 8:05 AM Luke Chen <show...@gmail.com> wrote: > > > > > > > >> Hi Kamal, > > > >> > > > >> Thanks for the response. > > > >> > > > >> I saw we added some new configs/metrics. Comments: > > > >> > > > >> LC5: Do you think this is a commonly happened issue that we need to > > add > > > a > > > >> separate `remote.log.offset.reader.threads` for it? > > > >> I thought this rarely happened. If so, maybe the additional > > > config/metrics > > > >> are not necessary? It makes the config more complicated. > > > >> > > > >> LC6: The config name: > > > >> `remote.log.offset.read.max.pending.tasks` , should we be consistent > > to > > > >> use > > > >> `reader`, instead of `read`? > > > >> > > > >> LC7: We should set a default value for the newly introduced configs > > and > > > >> written in KIP. > > > >> > > > >> Thanks. > > > >> Luke > > > >> > > > >> On Tue, Aug 13, 2024 at 8:47 PM Kamal Chandraprakash < > > > >> kamal.chandraprak...@gmail.com> wrote: > > > >> > > > >> > Hi Luke, > > > >> > > > > >> > Thanks for the review! > > > >> > > > > >> > > LC2: > > > >> > a. If the time taken to process the request is less than 5 mins, > > then > > > >> the > > > >> > Admin client will get a response. > > > >> > b. If the time taken to process the request is more than 5 mins, > > then > > > >> the > > > >> > Admin client will itself timeout the request due to the > > > >> > default-api-timeout. > > > >> > c. If the time taken to process the request is more than 6 mins, > > then > > > >> > the server will cancel the request in the DelayedRemoteListOffsets > > > >> > purgatory (to be implemented) and > > > >> > send TimeoutException back to the client if the client is > > waiting > > > >> for > > > >> > the response. > > > >> > > > > >> > > LC3: > > > >> > Updated the KIP. > > > >> > > > > >> > > LC4: > > > >> > The consumer retries the LIST_OFFSETS request incase of > > > failures/timeout > > > >> > but not the AdminClient. So, I think this is a retry feature in the > > > >> > Consumer. > > > >> > > > > >> > Updated the "Public Interfaces" section in the KIP > > > >> > < > > > >> > > > > >> > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1075%3A+Introduce+delayed+remote+list+offsets+purgatory+to+make+LIST_OFFSETS+async > > > >> > > > > > >> > by adding more details. PTAL. > > > >> > > > > >> > Thanks, > > > >> > Kamal > > > >> > > > > >> > > > > >> > On Tue, Aug 13, 2024 at 2:03 PM Luke Chen <show...@gmail.com> > > wrote: > > > >> > > > > >> > > Hi Kamal, > > > >> > > > > > >> > > Thanks for the update. > > > >> > > LC1: I see. Thanks. > > > >> > > LC2: What I still don't understand is what is the relationship > > > between > > > >> > > remote.list.offsets.request.timeout.ms V.S. > > > >> > > request.timeout/default.api.timeout. > > > >> > > Suppose we set request timeout to 30 seconds, > > default.api.timeout=5 > > > >> mins > > > >> > > and remote.list.offsets.request.timeout.ms = 6 mins. > > > >> > > So, when Admin sends a list offset request that needs to query > > > remote > > > >> > > storage, when will it throw timeout exception? 30 secs or 5 mins > > or > > > 6 > > > >> > mins? > > > >> > > We might need to make it clear in the KIP. > > > >> > > > > > >> > > LC3: > > > >> > > "Admin sends only one request and wait for upto > > default-api-timeout. > > > >> (eg) > > > >> > > If the admin is configured with default-api-timeout as 5 mins and > > > >> > > request-timeout as 30 seconds. And, the server takes 50 seconds to > > > >> > process > > > >> > > the LIST_OFFSETS request, then the admin sends only one > > LIST_OFFSETS > > > >> > > request, then receives the request from server after 50 seconds." > > > >> > > > > > >> > > In the end of the section, it should be receives the "response" > > from > > > >> > > server? > > > >> > > > > > >> > > LC4: I found the different consumer and admin behavior when > > setting > > > >> > > "request.timeout" and "default.api.timeout" is confusing. Are they > > > >> > expected > > > >> > > or a bug? > > > >> > > > > > >> > > Thank you. > > > >> > > Luke > > > >> > > > > > >> > > > > > >> > > On Thu, Aug 8, 2024 at 10:06 PM Kamal Chandraprakash < > > > >> > > kamal.chandraprak...@gmail.com> wrote: > > > >> > > > > > >> > > > Hi Luke, > > > >> > > > > > > >> > > > Thanks for the review! > > > >> > > > > > > >> > > > LC1: When the consumer starts to read data, then it might need > > the > > > >> > below > > > >> > > > offsets: > > > >> > > > earliest, latest, and last-committed-offset based on the > > > >> > > > "auto.offset.reset" config. > > > >> > > > > > > >> > > > The earliest and latest offsets have special timestamps -2 and > > -1, > > > >> > those > > > >> > > > timestamp corresponding offsets are cached in the > > > >> > > > broker memory and get served immediately. The > > > last-committed-offset > > > >> is > > > >> > > also > > > >> > > > cached in the GroupMetadata and > > > >> > > > gets served in the "OFFSET_FETCH" request. Unless the consumer > > > >> > > > explicitly uses the KafkaConsumer#offsetForTimes API, > > > >> > > > there won't be any delay in serving the data from the local log. > > > >> > > > > > > >> > > > In this KIP, we are trying to address the case in which multiple > > > >> > > consumers > > > >> > > > start at the same time and use the 'offsetForTimes` LIST_OFFSETS > > > >> API, > > > >> > > > assuming the remote requests are slow, then it should not block > > > >> other > > > >> > > > PRODUCE/FETCH requests. > > > >> > > > > > > >> > > > LC2: Sorry for the confusion. We were planning to introduce only > > > the > > > >> > > broker > > > >> > > > config "remote.list.offsets.timeout.ms". > > > >> > > > If we add the timeout in the ListOffsetsRequest.json, then when > > > old > > > >> > > clients > > > >> > > > talk with the new broker, we don't have > > > >> > > > a timeout to set on the server. Moved adding the timeout to the > > > >> > > > ListOffsetsRequest to the rejected alternatives section. > > > >> > > > > > > >> > > > -- > > > >> > > > Kamal > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > > > >
