smjn commented on PR #22601: URL: https://github.com/apache/kafka/pull/22601#issuecomment-4801662772
> > > @smjn I was expecting a single readLog method in LogReader which abstracts whether to fetch from local log or remote, it just provides a CompletableFuture. Do you think that can be achieved instead of two methods in LogReader? > > > > > > @apoorvmittal10 The current `read` method needs to stay until `DelayedShareFetch` is updated to use current `readAsync`. Post that we could refactor log reader - I would recommend 2 methods > > `read(options)` which always fetches remote and `read(options, boolean fetchRemote)` for flexibility, both returning completable futures. The `readRemote` method in the impl class is private. > > Post merge of this PR, and updating `DelayedShareFetch`, we will raise a minor PR to update `LogReader` with aforementioned API. > > Sounds good but why not to just have `read(options, boolean fetchRemote)` method alone. So DelayedShareFetch will use `false` as `fetchRemote` for now and DLQ will use as true. Later, I ll integrate the DelayedShareFetch correctly? @apoorvmittal10 > I can integrate the new changes in DelayedShareFetch but for this PR we should abstract the read from local or remote behind the readLog method itself. The suggestion is in line with your previous comment. Since I am not updating the `DelayedFetchFetch` as part of this PR, I cannot make the single method change here or `DelayedShareFetch` will break. We can move to single `read(options, boolean fetchRemote)` after `DelayedShareFetch` is updated with handling future based response. To distinguish completable future return type the method is transiently named `readAsyc`. Will refactor to read - post `DelayedShareFetch` change. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
