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: Track size in InMemoryKeyValueStore #7177

Merged
merged 2 commits into from Aug 13, 2019

Conversation

@ableegoldman
Copy link
Contributor

commented Aug 7, 2019

InMemoryKeyValueStore uses ConcurrentSkipListMap#size which takes linear time as it iterates over the entire map. We should just track size ourselves for approximateNumEntries

Should be cherry-picked back to 2.3

@ableegoldman

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

@mjsax

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

Should we target this to the same ticket that reported the regression (even if this is not a regression as the store was newly added)?

@mjsax mjsax added the streams label Aug 7, 2019

@ableegoldman ableegoldman changed the title MINOR: Track size in InMemoryKeyValueStore KAFKA-8736: Track size in InMemoryKeyValueStore Aug 8, 2019

@mjsax
Copy link
Member

left a comment

It seem the counting would be exact? Only issue, that we should never hit in practice is that size overflows?

...java/org/apache/kafka/streams/state/internals/InMemoryKeyValueStore.java Outdated Show resolved Hide resolved
@ableegoldman

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Yeah, although we weren't really guarding against that with the earlier TreeMap. I think it's reasonable to put that check on the user (unless we're concerned it might grow so large/fast it would wrap around to positive again and they'd miss it. Which seems...highly unlikely)

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

left a comment

LGTM.

@ableegoldman

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Java 11/12 failed with
org.apache.kafka.connect.integration.ExampleConnectIntegrationTest.testSourceConnector
org.apache.kafka.connect.integration.RebalanceSourceConnectorsIntegrationTest.testReconfigConnector
kafka.server.DescribeLogDirsRequestTest.testDescribeLogDirsRequest

Java 8 failed with
kafka.api.DelegationTokenEndToEndAuthorizationTest.testNoDescribeProduceOrConsumeWithoutTopicDescribeAcl
kafka.server.DescribeLogDirsRequestTest.testDescribeLogDirsRequest

Java 11/13 passed, retest this please

@@ -35,6 +35,7 @@
private final String name;
private final ConcurrentNavigableMap<Bytes, byte[]> map = new ConcurrentSkipListMap<>();
private volatile boolean open = false;
private long size = 0L; // SkipListMap#size is O(N) so we just do our best to track it

This comment has been minimized.

Copy link
@guozhangwang

guozhangwang Aug 8, 2019

Contributor

The reason why ConcurrentSkipListMap did not implement it in this way is because it is designed to be async, and we need it that way to support IQ / stream thread concurrency.

But since only stream thread would put / delete into the map (assuming we forbid iterator.remove from the other PR so it is not allowed from IQ), then it is okay to have this field as non-volatile.

This comment has been minimized.

Copy link
@ableegoldman

ableegoldman Aug 8, 2019

Author Contributor

Yep, that was the reasoning 😄

@mjsax

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Java 8: kafka.server.DescribeLogDirsRequestTest.testDescribeLogDirsRequest
Java 11 / 2.13:

kafka.server.DescribeLogDirsRequestTest.testDescribeLogDirsRequest
org.apache.kafka.connect.integration.RebalanceSourceConnectorsIntegrationTest.testReconfigConnector

Java 11 / 2.11 kafka.server.DescribeLogDirsRequestTest.testDescribeLogDirsRequest

No 3 clean runs for kafka.server.DescribeLogDirsRequestTest.testDescribeLogDirsRequest yet.

Retest this please.

@mjsax

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Java 11 / 2.12 failed with testDescribeLogDirsRequest (already fixed).

Java 11 / 2.13 timed out.

Java 8: TopicCommandWithAdminClientTest.testDescribeReportOverriddenConfigs

@ableegoldman Can you rebase this PR to get the fix for testDescribeLogDirsRequest?

@ableegoldman ableegoldman force-pushed the ableegoldman:IMKVStore-size branch to 71ceb47 Aug 9, 2019

@mjsax

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

Java 11 / 2.12: SaslOAuthBearerSslEndToEndAuthorizationTest.testNoDescribeProduceOrConsumeWithoutTopicDescribeAcl

Java 11 / 2.13 and Java8 passed.

Retest this please.

@ableegoldman

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

Java 11/12 passed, Java 11/13 failed with one failure, Java 8 failed with 0 failures (??). Test results no longer available, retest this, please

@bbejeck

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

Java 11/2.13 passed in one run and Java 11/2.11 passed in another Java 8 passed so merging this.

@bbejeck bbejeck merged commit f8078f3 into apache:trunk Aug 13, 2019

2 of 3 checks passed

JDK 11 and Scala 2.13 FAILURE 11798 tests run, 77 skipped, 1 failed.
Details
JDK 11 and Scala 2.12 SUCCESS 11798 tests run, 77 skipped, 0 failed.
Details
JDK 8 and Scala 2.11 SUCCESS 11798 tests run, 77 skipped, 0 failed.
Details
@bbejeck

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

Merged #7177 into trunk

bbejeck added a commit that referenced this pull request Aug 13, 2019
KAFKA-8736: Track size in InMemoryKeyValueStore (#7177)
InMemoryKeyValueStore uses ConcurrentSkipListMap#size which takes linear time as it iterates over the entire map. We should just track size ourselves for approximateNumEntries

Reviewers: Guozhang Wang <wangguoz@gmail.com>, Matthias J. Sax <mjsax@apache.org>
@bbejeck

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

cherry-picked to 2.3

xiowu0 added a commit to linkedin/kafka that referenced this pull request Aug 22, 2019
[LI-CHERRY-PICK] [f81189c] KAFKA-8736: Track size in InMemoryKeyValue…
…Store (apache#7177)

TICKET = KAFKA-8736
LI_DESCRIPTION =
EXIT_CRITERIA = HASH [f81189c]
ORIGINAL_DESCRIPTION =

InMemoryKeyValueStore uses ConcurrentSkipListMap#size which takes linear time as it iterates over the entire map. We should just track size ourselves for approximateNumEntries

Reviewers: Guozhang Wang <wangguoz@gmail.com>, Matthias J. Sax <mjsax@apache.org>
(cherry picked from commit f81189c)
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.