Skip to content

KAFKA-7012: Don't process SSL channels without data to process#5237

Merged
rajinisivaram merged 2 commits intoapache:trunkfrom
rajinisivaram:KAFKA-7012-ssl-buffered
Jun 19, 2018
Merged

KAFKA-7012: Don't process SSL channels without data to process#5237
rajinisivaram merged 2 commits intoapache:trunkfrom
rajinisivaram:KAFKA-7012-ssl-buffered

Conversation

@rajinisivaram
Copy link
Copy Markdown
Contributor

@rajinisivaram rajinisivaram commented Jun 15, 2018

Avoid unnecessary processing of SSL channels when there are some bytes buffered, but not enough to make progress.

Committer Checklist (excluded from commit message)

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

@rajinisivaram rajinisivaram requested review from ijuma and junrao June 15, 2018 23:16
@rajinisivaram
Copy link
Copy Markdown
Contributor Author

rajinisivaram commented Jun 15, 2018

@radai-rosenblatt @junrao @ijuma Can you review please? Thank you!

radai-rosenblatt
radai-rosenblatt reviewed Jun 18, 2018
View reviewed changes
Comment thread clients/src/test/java/org/apache/kafka/common/network/SslSelectorTest.java Outdated
selector.poll(5);
}

// Verify that pollSelectionKeys was invoked at least once once to process buffered data
Copy link
Copy Markdown
Contributor

@radai-rosenblatt radai-rosenblatt Jun 18, 2018

Choose a reason for hiding this comment

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

nit - "once once"

radai-rosenblatt
radai-rosenblatt reviewed Jun 18, 2018
View reviewed changes
Comment thread clients/src/main/java/org/apache/kafka/common/network/SslTransportLayer.java Outdated
netReadBuffer = Utils.ensureCapacity(netReadBuffer, netReadBufferSize());
if (netReadBuffer.remaining() > 0)
netread = readFromSocketChannel();
if (netread > 0)
Copy link
Copy Markdown
Contributor

@radai-rosenblatt radai-rosenblatt Jun 18, 2018

Choose a reason for hiding this comment

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

performance nit - this if can be moved inside the previous if, for some infinitesimal performance gain?

radai-rosenblatt
radai-rosenblatt approved these changes Jun 18, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@radai-rosenblatt radai-rosenblatt left a comment

Choose a reason for hiding this comment

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

LGTM

junrao
junrao approved these changes Jun 18, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@junrao junrao left a comment

Choose a reason for hiding this comment

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

@rajinisivaram : Thanks for the patch. LGTM. Just a minor question below.

Comment thread clients/src/test/java/org/apache/kafka/common/network/SslSelectorTest.java Outdated
// Verify that pollSelectionKeys was invoked at least once once to process buffered data
// but not more than once again since there isn't sufficient data to process.
assertTrue("Buffered data not processed: " + node1Polls, node1Polls.get() >= 1);
assertTrue("Too many calls to pollSelectionKeys: " + node1Polls, node1Polls.get() <= 2);
Copy link
Copy Markdown
Contributor

@junrao junrao Jun 18, 2018

Choose a reason for hiding this comment

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

Hmm, why would node1 get polled a second time? It seems that after the first poll, it would notice that it's not making progress?

Copy link
Copy Markdown
Contributor Author

@rajinisivaram rajinisivaram Jun 19, 2018

Choose a reason for hiding this comment

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

I was just being over-cautious just in case. But you are right, it shouldn't get polled a second time. I have updated the test.

@rajinisivaram
Copy link
Copy Markdown
Contributor Author

rajinisivaram commented Jun 19, 2018

@radai-rosenblatt @junrao Thank you both for the reviews. I have made the updates. I will merge to trunk and 2.0 after builds complete. I also ran system tests yesterday and there weren't any failures in the tests using SSL (there were connect and compatibilty test failures, which are known issues).

@rajinisivaram rajinisivaram force-pushed the KAFKA-7012-ssl-buffered branch from 6dad19f to b697678 Compare June 19, 2018 10:30
@rajinisivaram
Copy link
Copy Markdown
Contributor Author

rajinisivaram commented Jun 19, 2018

Test failure looks unrelated, merging to trunk and 2.0.

@rajinisivaram rajinisivaram merged commit e8955f7 into apache:trunk Jun 19, 2018
rajinisivaram added a commit that referenced this pull request Jun 19, 2018
@rajinisivaram
KAFKA-7012: Don't process SSL channels without data to process (#5237)
6020173 
Avoid unnecessary processing of SSL channels when there are some bytes buffered, but not enough to make progress.

Reviewers: Radai Rosenblatt <radai.rosenblatt@gmail.com>, Jun Rao <junrao@gmail.com>
rajinisivaram added a commit that referenced this pull request Jun 20, 2018
@rajinisivaram
KAFKA-7012: Don't process SSL channels without data to process (#5237)
2ea3e55 
Avoid unnecessary processing of SSL channels when there are some bytes buffered, but not enough to make progress.

Reviewers: Radai Rosenblatt <radai.rosenblatt@gmail.com>, Jun Rao <junrao@gmail.com>
rajinisivaram added a commit that referenced this pull request Jun 20, 2018
@rajinisivaram
KAFKA-7012: Don't process SSL channels without data to process (#5237)
ad9de15 
Avoid unnecessary processing of SSL channels when there are some bytes buffered, but not enough to make progress.

Reviewers: Radai Rosenblatt <radai.rosenblatt@gmail.com>, Jun Rao <junrao@gmail.com>
ying-zheng pushed a commit to ying-zheng/kafka that referenced this pull request Jul 6, 2018
@rajinisivaram @ying-zheng
KAFKA-7012: Don't process SSL channels without data to process (apach…
2c26976 
…e#5237)

Avoid unnecessary processing of SSL channels when there are some bytes buffered, but not enough to make progress.

Reviewers: Radai Rosenblatt <radai.rosenblatt@gmail.com>, Jun Rao <junrao@gmail.com>
allenxwang pushed a commit to allenxwang/kafka that referenced this pull request Aug 24, 2018
Merge pull request apache#77 in DATA_PIPELINE/kafka from ~AWANG/kafka…
8d3ddf2 
…:1.1.1-sync to 1.1-nflx

* commit '9611672e287c1a7933a78590e3f381da2ae7d136': (57 commits)
  MINOR: increase dev version from 1.1.1-SNAPSHOT to 1.1.2-SNAPSHOT (apache#5409)
  MINOR: Add thread dumps if broker node cannot be stopped (apache#5373)
  MINOR: update release.py
  MINOR: fix upgrade docs for Streams (apache#5392)
  MINOR: improve docs version numbers (apache#5372)
  Update version on the branch to 1.1.2-SNAPSHOT
  KAFKA-6292; Improve FileLogInputStream batch position checks to avoid type overflow (apache#4928)
  HOTFIX: Fix checkstyle errors in MetricsTest (apache#5345)
  KAFKA-7136: Avoid deadlocks in synchronized metrics reporters (apache#5341)
  MINOR: Close timing window in SimpleAclAuthorizer startup (apache#5318)
  MINOR: Use kill_java_processes when killing ConsoleConsumer in system tests (apache#5297)
  KAFKA-7104: More consistent leader's state in fetch response (apache#5305)
  Revert "MINOR: Avoid coarse lock in Pool#getAndMaybePut (apache#5258)"
  MINOR: Avoid coarse lock in Pool#getAndMaybePut (apache#5258)
  MINOR: bugfix streams total metrics (apache#5277)
  KAFKA-7082: Concurrent create topics may throw NodeExistsException (apache#5259)
  MINOR: Upgrade to Gradle 4.8.1
  KAFKA-7012: Don't process SSL channels without data to process (apache#5237)
  KAFKA-7058: Comparing schema default values using Objects#deepEquals()
  KAFKA-7047: Added SimpleHeaderConverter to plugin isolation whitelist
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@junrao junrao junrao approved these changes

@ijuma ijuma Awaiting requested review from ijuma

+1 more reviewer

@radai-rosenblatt radai-rosenblatt radai-rosenblatt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rajinisivaram @junrao @radai-rosenblatt