kirktrue commented on code in PR #14359:
URL: https://github.com/apache/kafka/pull/14359#discussion_r1323772849
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java:
##########
@@ -50,7 +55,8 @@
public class Fetcher<K, V> extends AbstractFetch<K, V> {
private final Logger log;
- private final AtomicBoolean isClosed = new AtomicBoolean(false);
+ private final FetchCollector<K, V> fetchCollector;
+ private final ConsumerNetworkClient client;
Review Comment:
I agree, it's ugly 😦
`client` and `nodeStatusDetector` both point at the same object in `Fetcher`
and `AbstractFetch`, respectively, but the two classes use them for different
purposes.
The `ConsumerNetworkClient` used in the `Fetcher` invokes methods like
`poll()`, `send()`, and `disableWakeups()` to perform network I/O "directly."
The forthcoming `FetchRequestManager` adheres to the design principle that
request managers don't perform network I/O directly. To enforce that, we
introduced this intentionally limited `NodeStatusDetector` to allow for
checking basic state of nodes.
I'll keep looking at it a bit to see if I can come up with something cleaner
because I don't like it either.
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java:
##########
@@ -50,7 +55,8 @@
public class Fetcher<K, V> extends AbstractFetch<K, V> {
private final Logger log;
- private final AtomicBoolean isClosed = new AtomicBoolean(false);
+ private final FetchCollector<K, V> fetchCollector;
+ private final ConsumerNetworkClient client;
Review Comment:
I agree, it's ugly 😦
`client` and `nodeStatusDetector` both point at the same object in `Fetcher`
and `AbstractFetch`, respectively, but the two classes use them for different
purposes.
The `ConsumerNetworkClient` used in the `Fetcher` invokes methods like
`poll()`, `send()`, and `disableWakeups()` to perform network I/O "directly."
The forthcoming `FetchRequestManager` adheres to the design principle that
request managers don't perform network I/O directly. To enforce that, we
introduced this intentionally limited `NodeStatusDetector` to allow for
checking basic state of nodes.
I'll keep looking at it a bit to see if I can come up with something cleaner
because I don't like it either.
--
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
