Skip to content

KAFKA-12427: Don't update connection idle time for muted connections#10267

Merged
rajinisivaram merged 3 commits intoapache:trunkfrom
splett2:KAFKA-12427
Mar 16, 2021
Merged

KAFKA-12427: Don't update connection idle time for muted connections#10267
rajinisivaram merged 3 commits intoapache:trunkfrom
splett2:KAFKA-12427

Conversation

@splett2
Copy link
Copy Markdown
Contributor

@splett2 splett2 commented Mar 5, 2021

Selector.poll() will always call pollSelectionKeys() for channels with buffered data. pollSelectionKeys() will always update connection last idle time, even if the channel is muted and we don't actually read from the channel.

There is an existing unit test idleExpiryWithBufferedReceives that fails to catch this behavior because the MockTime object used in the test is updated in a large enough increment to expire a connection between calls to poll(). After updating the test to advance time in smaller increments, the test fails without the Selector change.

Committer Checklist (excluded from commit message)

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

@splett2
Copy link
Copy Markdown
Contributor Author

splett2 commented Mar 5, 2021

cc @rajinisivaram for reviews/thoughts

This approach is fairly simple, and we still end up calling pollSelectionKeys() with muted channels. An alternative that I was thinking of would be to remove a kafka channel from keysWithBufferedReads when explicitly muting, and then checking whether the channel has buffered data when unmuting.

@rajinisivaram
Copy link
Copy Markdown
Contributor

@splett2 Thanks for the PR. I think it would be better to store channels in keysWithBufferedReads only when the channel is not explicitly muted since we care about this state only when we are ready to read from the channel. So we could check if channel is explicitly muted when adding to keysWithBufferedReads and add the key to keysWithBufferedReads when unmuting. This would avoid processing the channel unnecessarily.

// register all per-connection metrics at once
sensors.maybeRegisterConnectionMetrics(nodeId);
if (idleExpiryManager != null)
if (idleExpiryManager != null && !explicitlyMutedChannels.contains(channel))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is limiting the times when we perform the update. For example when sending response, the channel is active, but explicitly muted. Don't think we want to skip update in this case.

@splett2
Copy link
Copy Markdown
Contributor Author

splett2 commented Mar 8, 2021

@rajinisivaram
thanks for the review, that makes sense to me.

Copy link
Copy Markdown
Contributor

@rajinisivaram rajinisivaram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@splett2 Thanks for the update, LGTM. Merging to trunk.

@rajinisivaram rajinisivaram merged commit bf63990 into apache:trunk Mar 16, 2021
ijuma added a commit to confluentinc/kafka that referenced this pull request Mar 17, 2021
Conflicts:
* Jenkinsfile: `install` -> `publishToMavenLocal`, drop ARM build and
other changes that don't make sense for Confluent's version of
`Jenkinsfile`.
* build.gradle: keep Confluent changes for automatic skipping signing
for specific version patterns (upstream only does it if the version ends
with `SNAPSHOT`).

Commits:
* apache-github/trunk: (59 commits)
  MINOR: Remove redundant allows in import-control.xml (apache#10339)
  MINOR: remove some specifying types in tool command (apache#10329)
  KAFKA-12455: Fix OffsetValidationTest.test_broker_rolling_bounce failure with Raft (apache#10322)
  MINOR: Add toString to various Kafka Metrics classes (apache#10330)
  KAFKA-12330; FetchSessionCache may cause starvation for partitions when FetchResponse is full (apache#10318)
  KAFKA-12427: Don't update connection idle time for muted connections (apache#10267)
  MINOR; Various code cleanups (apache#10319)
  HOTFIX: timeout issue in removeStreamThread() (apache#10321)
  revert stream logging level back to ERROR (apache#10320)
  KAFKA-12352: Make sure all rejoin group and reset state has a reason (apache#10232)
  KAFKA-10348: Share client channel between forwarding and auto creation manager (apache#10135)
  MINOR: Update year in NOTICE (apache#10308)
  KAFKA-12398: Fix flaky test `ConsumerBounceTest.testClose` (apache#10243)
  MINOR: Remove redundant inheritance from FilteringJmxReporter #onMetricRemoved (apache#10303)
  KAFKA-12462: proceed with task revocation in case of thread in PENDING_SHUTDOWN (apache#10311)
  KAFKA-12460; Do not allow raft truncation below high watermark (apache#10310)
  MINOR: Log project, gradle, java and scala versions at the start of the build (apache#10307)
  KAFKA-10357: Add missing repartition topic validation (apache#10305)
  MINOR: Improve error message in MirrorConnectorsIntegrationBaseTest (apache#10268)
  MINOR: Add missing unit tests for Mirror Connect (apache#10192)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants