[
https://issues.apache.org/jira/browse/KAFKA-48?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13256073#comment-13256073
]
Joel Koshy commented on KAFKA-48:
---------------------------------
+1 on the patch. I have a few minor comments:
KafkaRequestHandlers :
- requestLogger unused.
ConsumerConfig:
- maxFetchWait -> rename the prop to max.fetch.wait.ms and the val to
maxFetchWaitMs
- Can we get rid of fetcherBackoffMs? It says it is deprecated, but had a
reference in FetcherRunnable which you removed.
- May want to have an explicit constraint that consumerTimeoutMs <=
maxFetchWait
RequestPurgatory:
- Unused import.
- The parameterized types and overall tricky nature of this component make
it somewhat difficult to follow. I (think) I understood it only after
looking at its usage in KafkaApis, so the comments and javadocs (including
class' summary on top) can only go so far. Even so, I think the comments
seem slightly out of sync with the code and can be improved a bit. E.g.,
what is "given size" in the update method's comment? current keys in the
comment for watch == the given request's keys. and so on.
- Also, it may be easier to follow if we do some renaming, but it's a matter
of taste and I may have misunderstood the code to begin with:
- I find it confusing that there's a map called watchers which is a map
from keys to Watcher objects, and the Watcher class itself has a
linked-list of delayed requests called watchers. May be unwieldy, but
how about renaming:
- RequestPurgatory.watchers to watchedRequestsForKey
- Watchers to WatchedRequests
- Watchers.watchers to requests
- Rename DelayedRequest.satisfied to satisfiedOrExpired (I find it weird
that the reaper marks expired requests as satisfied.)
- update -> maybeNotify?
- In collectSatisfiedRequests, the comment on "another thread has satisfied
this request". That can only be the ExpiredRequestReaper thread right?
- It is slightly odd that we have to call the reaper's satisfyRequest method
from Watcher. Would it work to move the unsatisfied counter up to
RequestPurgatory?
> Implement optional "long poll" support in fetch request
> -------------------------------------------------------
>
> Key: KAFKA-48
> URL: https://issues.apache.org/jira/browse/KAFKA-48
> Project: Kafka
> Issue Type: Bug
> Reporter: Jun Rao
> Assignee: Jay Kreps
> Attachments: KAFKA-48-v2.patch, KAFKA-48-v3.patch, KAFKA-48-v4.patch,
> KAFKA-48.patch, kafka-48-v3-to-v4-changes.diff
>
>
> Currently, the fetch request is non-blocking. If there is nothing on the
> broker for the consumer to retrieve, the broker simply returns an empty set
> to the consumer. This can be inefficient, if you want to ensure low-latency
> because you keep polling over and over. We should make a blocking version of
> the fetch request so that the fetch request is not returned until the broker
> has at least one message for the fetcher or some timeout passes.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
