Skip to content

KAFKA-9091: Add a metric tracking the number of open connections with a given SSL cipher type#7588

Merged
cmccabe merged 4 commits into
apache:trunkfrom
cmccabe:KAFKA-9091
Dec 6, 2019
Merged

KAFKA-9091: Add a metric tracking the number of open connections with a given SSL cipher type#7588
cmccabe merged 4 commits into
apache:trunkfrom
cmccabe:KAFKA-9091

Conversation

@cmccabe

@cmccabe cmccabe commented Oct 23, 2019

Copy link
Copy Markdown
Contributor

No description provided.

@tombentley tombentley left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, but a couple of question for my own benefit.

Comment thread clients/src/main/java/org/apache/kafka/common/network/SslTransportLayer.java Outdated
Comment thread clients/src/main/java/org/apache/kafka/common/metrics/IntGaugeSuite.java Outdated
@cmccabe

cmccabe commented Nov 4, 2019

Copy link
Copy Markdown
Contributor Author

retest this please

@cmccabe cmccabe force-pushed the KAFKA-9091 branch 2 times, most recently from 59387bc to 832a6f4 Compare November 11, 2019 20:54

@ijuma ijuma left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few more comments. Also, can we please add tests?

Comment thread clients/src/main/java/org/apache/kafka/common/metrics/IntGaugeSuite.java Outdated
Comment thread clients/src/main/java/org/apache/kafka/common/metrics/IntGaugeSuite.java Outdated
Comment thread clients/src/main/java/org/apache/kafka/common/metrics/IntGaugeSuite.java Outdated
Comment thread clients/src/main/java/org/apache/kafka/common/metrics/IntGaugeSuite.java Outdated
Comment thread clients/src/main/java/org/apache/kafka/common/network/SslInformation.java Outdated
@cmccabe cmccabe force-pushed the KAFKA-9091 branch 2 times, most recently from 5cd95b5 to dea1270 Compare November 19, 2019 19:38
Comment thread clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java Outdated
@cmccabe cmccabe force-pushed the KAFKA-9091 branch 2 times, most recently from 24d02c5 to 423edcc Compare November 19, 2019 23:08
@dajac

dajac commented Nov 26, 2019

Copy link
Copy Markdown
Member

@cmccabe How far are you from merging this one? I'd like to reuse your IntGaugeSuite.

Comment thread clients/src/main/java/org/apache/kafka/common/network/SslTransportLayer.java Outdated

@ijuma ijuma left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for adding the test and other updates. A few comments/questions below.

Comment thread clients/src/main/java/org/apache/kafka/common/metrics/IntGaugeSuite.java Outdated
Comment thread clients/src/main/java/org/apache/kafka/common/network/SslInformation.java Outdated

@rajinisivaram rajinisivaram left a comment

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.

@cmccabe Thanks for the PR, looks good. Left a few minor comments. The KIP seems to suggest that the new metric is only on the broker (it shows listener name etc.), while I think we are adding the metric on both client-side and server-side. May be worth updating the KIP to reflect that.

Comment thread clients/src/main/java/org/apache/kafka/common/metrics/IntGaugeSuite.java Outdated
Comment thread clients/src/main/java/org/apache/kafka/common/metrics/IntGaugeSuite.java Outdated
Comment thread clients/src/main/java/org/apache/kafka/common/metrics/IntGaugeSuite.java Outdated
Comment thread clients/src/main/java/org/apache/kafka/common/network/SslTransportLayer.java Outdated
Comment thread clients/src/test/java/org/apache/kafka/common/network/SslSelectorTest.java Outdated
Comment thread clients/src/test/java/org/apache/kafka/common/network/SslSelectorTest.java Outdated

@rajinisivaram rajinisivaram left a comment

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.

@cmccabe Thanks for the updates, LGTM. Left a minor comment, but it should be ok to leave it as-is too.


import java.util.Objects;

public class CipherInformation {

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.

Still not sure about the name since this already includes more than just cipher. But since it is an internal class, it should be ok to leave it as is for now.

@cmccabe cmccabe merged commit fa41521 into apache:trunk Dec 6, 2019
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.

5 participants