Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-8736: Streams performance improvement, use isEmpty() rather tha… #7164

Merged
merged 1 commit into from Aug 6, 2019

Conversation

@mjarvie
Copy link
Contributor

commented Aug 5, 2019

…n size() == 0

According to https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentSkipListMap.html#size--, the size method has to traverse all elements to get a count. It looks like the count is being compared against 0 to determine if the map is empty; In this case, we don't need a full count. Instead, the isEmpty() method should be used, which just looks for one node.

No expected changes to unit or integration tests.

Committer Checklist (excluded from commit message)

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

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

Thanks for the PR! LGTM.

I think this should be cherry-picked back to 2.3 cc/ @bbejeck @mjsax @guozhangwang

@mjsax mjsax added the streams label Aug 5, 2019

@mjsax
mjsax approved these changes Aug 5, 2019
Copy link
Member

left a comment

LGTM.

Waiting for Jenkins to finish before we can merge.

@guozhangwang
Copy link
Contributor

left a comment

Thanks @mjarvie ! LGTM.

@mjsax

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

Java 11 / 2.12 failed with kafka.server.DescribeLogDirsRequestTest.testDescribeLogDirsRequest
Java 11 / 2.13 failed with kafka.server.DescribeLogDirsRequestTest.testDescribeLogDirsRequest
Java 8 failed with:

org.apache.kafka.connect.integration.ExampleConnectIntegrationTest.testSourceConnector
org.apache.kafka.connect.integration.RebalanceSourceConnectorsIntegrationTest.testStartTwoConnectors
kafka.api.SaslSslAdminClientIntegrationTest.testIncrementalAlterConfigsForLog4jLogLevels
kafka.api.SaslSslAdminClientIntegrationTest.testIncrementalAlterConfigsForLog4jLogLevelsCanResetLoggerToCurrentRoot

Retest this please

@mjsax

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

Java 11 / 2.12: org.apache.kafka.connect.integration.RebalanceSourceConnectorsIntegrationTest.testDeleteConnector
Java 11 / 2.13: kafka.server.DescribeLogDirsRequestTest.testDescribeLogDirsRequest
Java 8: kafka.server.DescribeLogDirsRequestTest.testDescribeLogDirsRequest

Merging. Thanks for the PR @mjarvie

@mjsax mjsax merged commit 7ebcd50 into apache:trunk Aug 6, 2019

0 of 3 checks passed

JDK 11 and Scala 2.12 FAILURE 11796 tests run, 67 skipped, 1 failed.
Details
JDK 11 and Scala 2.13 FAILURE 11796 tests run, 67 skipped, 1 failed.
Details
JDK 8 and Scala 2.11 FAILURE 11796 tests run, 67 skipped, 1 failed.
Details
mjsax added a commit that referenced this pull request Aug 6, 2019
KAFKA-8736: Streams performance improvement, use isEmpty() rather tha…
…n size() == 0 (#7164)

Reviewers: A. Sophie Blee-Goldman <sophie@confluent.io>, Matthias J. Sax <matthias@confluent.io>, Guozhang Wang <guozhang@confluent.io>
mjsax added a commit that referenced this pull request Aug 6, 2019
KAFKA-8736: Streams performance improvement, use isEmpty() rather tha…
…n size() == 0 (#7164)

Reviewers: A. Sophie Blee-Goldman <sophie@confluent.io>, Matthias J. Sax <matthias@confluent.io>, Guozhang Wang <guozhang@confluent.io>
mjsax added a commit that referenced this pull request Aug 6, 2019
KAFKA-8736: Streams performance improvement, use isEmpty() rather tha…
…n size() == 0 (#7164)

Reviewers: A. Sophie Blee-Goldman <sophie@confluent.io>, Matthias J. Sax <matthias@confluent.io>, Guozhang Wang <guozhang@confluent.io>
mjsax added a commit that referenced this pull request Aug 6, 2019
KAFKA-8736: Streams performance improvement, use isEmpty() rather tha…
…n size() == 0 (#7164)

Reviewers: A. Sophie Blee-Goldman <sophie@confluent.io>, Matthias J. Sax <matthias@confluent.io>, Guozhang Wang <guozhang@confluent.io>
@mjsax

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

Merged to trunk and cherry-picked to 2.3, 2.2, 2.1, and 2.0.

ijuma added a commit to confluentinc/kafka that referenced this pull request Aug 8, 2019
Sync from apache/kafka (8 August 2019) and upgrade avro to 1.9.0
The avro upgrade was needed to fix the following error during
':support-metrics-client:generateAvro':

Caused by: org.apache.velocity.exception.MethodInvocationException: Variable $velocityCount has not been set at /org/apache/avro/compiler/specific/templates/java/classic/record.vm[line 82, column 150]

Conflicts:
 * gradle.properties -> trivial fix, `scalaVersion` is next to `version`, which
 is different in ccs kafka
 * gradle/dependencies.gradle -> reduce divergence with apache kafka by using
 `httpclient` instead of `httpcomponents` from the `versions` array. Remove
 unused `httpmime` and `httpcomponents` from said array.

* apache-github/trunk:
  MINOR: Update dependencies for Kafka 2.4 (apache#7126)
  KAFKA-8599: Use automatic RPC generation in ExpireDelegationToken
  MINOR: Upgrade jackson-databind to 2.9.9.3 (apache#7125)
  MINOR: some small style fixes to RoundRobinPartitioner
  KAFKA-8736: Streams performance improvement, use isEmpty() rather than size() == 0 (apache#7164)
  Minor: Refactor methods to add metrics to sensor in `StreamsMetricsImpl` (apache#7161)
xiowu0 added a commit to linkedin/kafka that referenced this pull request Aug 22, 2019
[LI-CHERRY-PICK] [0826d6e] KAFKA-8736: Streams performance improvemen…
…t, use isEmpty() rather than size() == 0 (apache#7164)

TICKET = KAFKA-8736
LI_DESCRIPTION =
EXIT_CRITERIA = HASH [0826d6e]
ORIGINAL_DESCRIPTION =

Reviewers: A. Sophie Blee-Goldman <sophie@confluent.io>, Matthias J. Sax <matthias@confluent.io>, Guozhang Wang <guozhang@confluent.io>
(cherry picked from commit 0826d6e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.