KAFKA-8635: Skip client poll in Sender loop when no request is sent by bob-barrett · Pull Request #7085 · apache/kafka

bob-barrett · 2019-07-12T22:01:43Z

This patch changes maybeSendTransactionalRequest to handle both sending and polling transactional requests (and renames it to maybeSendAndPollTransactionalRequest), and skips the call to poll if no request is actually sent. It also removes the inner loop inside maybeSendAndPollTransactionalRequest and relies on the main Sender loop for retries.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

This patch breaks up maybeSendTransactionalRequest() and changes it to return false if a FindCoordinatorRequest is enqueued. If this is the case, we no longer poll for because no request was actually sent.

hachikuji

Thanks for the patch. I think the fix make sense. I left a few cleanup suggestions for consideration.

hachikuji · 2019-07-13T07:29:54Z

+     */
+    private boolean maybeSendTransactionalRequest(TransactionManager.TxnRequestHandler nextRequestHandler) {
        AbstractRequest.Builder<?> requestBuilder = nextRequestHandler.requestBuilder();
        while (!forceClose) {


Not necessarily something we have to do here, but I think we should be able to get rid of this loop and just rely on the next iteration of runOnce() to handle retries.

hachikuji · 2019-07-13T07:30:54Z

+                TransactionManager.TxnRequestHandler nextRequestHandler = transactionManager.nextRequestHandler(accumulator.hasIncomplete());
+                if (nextRequestHandler != null) {
+                    if (maybeSendTransactionalRequest(nextRequestHandler)) {
+                        client.poll(retryBackoffMs, time.milliseconds());


This logic is a bit awkward. I guess it's because client.poll is disconnected from the send logic. I wonder if it would be reasonable to create a method like maybePollTransactionalRequest which handles both the inflight check, sending the next request, and calling client.poll if necessary.

hachikuji · 2019-07-13T07:34:30Z

+    private boolean maybeSendTransactionalRequest(TransactionManager.TxnRequestHandler nextRequestHandler) {
        AbstractRequest.Builder<?> requestBuilder = nextRequestHandler.requestBuilder();
        while (!forceClose) {
            Node targetNode = null;


Minor simplification we can do below:

if (targetNode == null || !awaitReady(client, targetNode, time, requestTimeoutMs)) { transactionManager.lookupCoordinator(nextRequestHandler); break; }

A helper for awaitReady might be useful as well. Might be a chance to consolidate the awaitLeastLoadedNodeReady path.

…elper method

hachikuji

Thanks, looks good. Just a couple more comments.

hachikuji · 2019-07-18T03:08:53Z


-    private Node awaitLeastLoadedNodeReady(long remainingTimeMs) throws IOException {
-        Node node = client.leastLoadedNode(time.milliseconds());
+    private Node awaitLeastLoadedNodeReady(long remainingTimeMs, FindCoordinatorRequest.CoordinatorType coordinatorType) throws IOException {


nit: the name should be changed since we do not always use the least loaded node.

I noticed we are always using request timeout for the argument. Maybe we can drop the argument?

hachikuji · 2019-07-18T03:23:39Z

            metadata.requestUpdate();
        }
+
+        transactionManager.lookupCoordinator(nextRequestHandler);


It reads a bit strange to fall through to lookupCoordinator if we know the request doesn't need the coordinator. Maybe clearer with a slight restructure:

transactionManager.retry(nextRequestHandler); if (nextRequestHandler.needsCoordinator()) { transactionManager.lookupCoordinator(nextRequestHandler); } else { // For non-coordinator requests, sleep here to prevent a tight loop when no node is available time.sleep(retryBackoffMs); metadata.requestUpdate(); }

hachikuji

LGTM. Thanks for the patch!

…7085) This patch changes maybeSendTransactionalRequest to handle both sending and polling transactional requests (and renames it to maybeSendAndPollTransactionalRequest), and skips the call to poll if no request is actually sent. It also removes the inner loop inside maybeSendAndPollTransactionalRequest and relies on the main Sender loop for retries. Reviewers: Jason Gustafson <jason@confluent.io>

* apache-github/2.3: MINOR: Update documentation for enabling optimizations (apache#7099) MINOR: Remove stale streams producer retry default docs. (apache#6844) KAFKA-8635; Skip client poll in Sender loop when no request is sent (apache#7085) KAFKA-8615: Change to track partition time breaks TimestampExtractor (apache#7054) KAFKA-8670; Fix exception for kafka-topics.sh --describe without --topic mentioned (apache#7094) KAFKA-8602: Separate PR for 2.3 branch (apache#7092) KAFKA-8530; Check for topic authorization errors in OffsetFetch response (apache#6928) KAFKA-8662; Fix producer metadata error handling and consumer manual assignment (apache#7086) KAFKA-8637: WriteBatch objects leak off-heap memory (apache#7050) KAFKA-8620: fix NPE due to race condition during shutdown while rebalancing (apache#7021) HOT FIX: close RocksDB objects in correct order (apache#7076) KAFKA-7157: Fix handling of nulls in TimestampConverter (apache#7070) KAFKA-6605: Fix NPE in Flatten when optional Struct is null (apache#5705) Fixes apache#8198 KStreams testing docs use non-existent method pipe (apache#6678) KAFKA-5998: fix checkpointableOffsets handling (apache#7030) KAFKA-8653; Default rebalance timeout to session timeout for JoinGroup v0 (apache#7072) KAFKA-8591; WorkerConfigTransformer NPE on connector configuration reloading (apache#6991) MINOR: add upgrade text (apache#7013) Bump version to 2.3.1-SNAPSHOT

…p when no request is sent (apache#7085) TICKET = KAFKA-8635 LI_DESCRIPTION = EXIT_CRITERIA = HASH [2371457] ORIGINAL_DESCRIPTION = This patch changes maybeSendTransactionalRequest to handle both sending and polling transactional requests (and renames it to maybeSendAndPollTransactionalRequest), and skips the call to poll if no request is actually sent. It also removes the inner loop inside maybeSendAndPollTransactionalRequest and relies on the main Sender loop for retries. Reviewers: Jason Gustafson <jason@confluent.io> (cherry picked from commit 2371457)

KAFKA-8635: Skip client poll in Sender loop when no request is sent

ddd2d3f

This patch breaks up maybeSendTransactionalRequest() and changes it to return false if a FindCoordinatorRequest is enqueued. If this is the case, we no longer poll for because no request was actually sent.

hachikuji reviewed Jul 13, 2019

View reviewed changes

PR feedback: consolidate awaitReady path, move in-flight check into h…

2da11bc

…elper method

hachikuji reviewed Jul 18, 2019

View reviewed changes

Rename awaitLeastLoadedNode, check before calling lookupCoordinator

0cd3376

hachikuji approved these changes Jul 18, 2019

View reviewed changes

hachikuji merged commit 2e26a46 into apache:trunk Jul 18, 2019

bob-barrett deleted the KAFKA-8635 branch July 18, 2019 22:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-8635: Skip client poll in Sender loop when no request is sent#7085

KAFKA-8635: Skip client poll in Sender loop when no request is sent#7085
hachikuji merged 3 commits intoapache:trunkfrom
bob-barrett:KAFKA-8635

bob-barrett commented Jul 12, 2019 •

edited

Loading

Uh oh!

hachikuji left a comment

Uh oh!

hachikuji Jul 13, 2019

Uh oh!

hachikuji Jul 13, 2019 •

edited

Loading

Uh oh!

hachikuji Jul 13, 2019

Uh oh!

hachikuji Jul 13, 2019

Uh oh!

hachikuji left a comment

Uh oh!

hachikuji Jul 18, 2019

Uh oh!

hachikuji Jul 18, 2019

Uh oh!

hachikuji left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

bob-barrett commented Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

hachikuji Jul 13, 2019

Choose a reason for hiding this comment

Uh oh!

hachikuji Jul 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hachikuji Jul 13, 2019

Choose a reason for hiding this comment

Uh oh!

hachikuji Jul 13, 2019

Choose a reason for hiding this comment

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

hachikuji Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

hachikuji Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bob-barrett commented Jul 12, 2019 •

edited

Loading

hachikuji Jul 13, 2019 •

edited

Loading