date:20231119

Re: [PR] MINOR: Expose earliest local timestamp via the GetOffsetShell [kafka]

2023-11-19 Thread via GitHub



kamalcph commented on code in PR #14788:
URL: https://github.com/apache/kafka/pull/14788#discussion_r1398724150


##
clients/src/main/java/org/apache/kafka/clients/admin/OffsetSpec.java:
##
@@ -26,6 +26,7 @@ public class OffsetSpec {
 public static class EarliestSpec extends OffsetSpec { }
 public static class LatestSpec extends OffsetSpec { }
 public static class MaxTimestampSpec extends OffsetSpec { }
+public static class EarliestLocalTimestampSpec extends OffsetSpec { }

Review Comment:
   Can we rename `EarliestLocalTimestampSpec` to `EarliestLocalSpec` similar to 
earliest and latest?



##
clients/src/main/java/org/apache/kafka/clients/admin/OffsetSpec.java:
##
@@ -70,4 +71,8 @@ public static OffsetSpec maxTimestamp() {
 return new MaxTimestampSpec();
 }
 
+public static OffsetSpec earliestLocalTimestamp() {

Review Comment:
   ditto:
   
   `earliestLocalTimestamp` -> `earliestLocal`
   



##
tools/src/main/java/org/apache/kafka/tools/GetOffsetShell.java:
##
@@ -281,14 +281,16 @@ private OffsetSpec parseOffsetSpec(String 
listOffsetsTimestamp) throws TerseExce
 return OffsetSpec.latest();
 case "max-timestamp":
 return OffsetSpec.maxTimestamp();
+case "earliest-local-timestamp":

Review Comment:
   ditto:
   
   `earliest-local-timestamp` -> `earliest-local`



-- 
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: jira-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] KAFKA-15327: ensure the commit manager commit on close [kafka]

2023-11-19 Thread via GitHub



philipnee commented on PR #14710:
URL: https://github.com/apache/kafka/pull/14710#issuecomment-1818294807

   Hi @lucasbru - Thank you so much for the time reviewing this PR.  Apologize 
for the unclarity, so I've updated the PR description as well as added some 
comments. Does it explain the intention of the PR better?


-- 
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: jira-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] KAFKA-15776: Use the FETCH request timeout as the delay timeout for DelayedRemoteFetch [kafka]

2023-11-19 Thread via GitHub

kamalcph commented on PR #14778:
URL: https://github.com/apache/kafka/pull/14778#issuecomment-1818291232

> Could you please help me understand how this change works with
fetch.max.wait.ms from a user perspective i.e. what happens when we are
retrieving data from both local & remote in a single fetch call?

`fetch.max.wait.ms` timeout is applicable only when there is no enough data
(`fetch.min.bytes`) to respond back to the client. This is a special case where
we are reading the data from both local and remote, the FETCH request has to
wait for the tail latency which is a combined latency of reading from both
local and remote storage.

Note that we always read from only one remote partition up-to
`max.partition.fetch.bytes` even-though there is available bandwidth in the
FETCH response (`fetch.max.bytes`) and the client rotates the partition order
in the next FETCH request so that next partitions are served.

> Also, wouldn't this change user clients? Asking because prior to this
change users were expecting a guaranteed response within fetch.max.wait.ms =
500ms but now they might not receive a response until 40s request.timeout.ms.
If the user has configured their application timeouts to according to
fetch.max.wait.ms, this change will break my application.

`fetch.max.wait.ms` doesn't guarantee a response within this timeout. The
client expires the request only when it exceeds the `request.timeout.ms` of 30
seconds (default). The time taken to serve the FETCH request can be higher than
the `fetch.max.wait.ms` due to slow hard-disk, sector errors in disk and so on.

The
[FetchRequest.json](https://sourcegraph.com/github.com/apache/kafka/-/blob/clients/src/main/resources/common/message/FetchRequest.json)
doesn't expose the client configured request timeout, so we are using the
default server request timeout of 30 seconds. Otherwise, we can introduce one
more config `fetch.remote.max.wait.ms` to define the delay timeout for
DelayedRemoteFetch requests. We need to decide whether to keep this config in
the client/server since the server operator may need to tune this config if the
remote storage degrades and latency to serve the FETCH requests is high.

--
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.

74 matches

Mail list logo