Skip to content

KAFKA-7918: Inline generic parameters Pt. II: RocksDB Bytes Store and Memory LRU Caches#6327

Merged
bbejeck merged 6 commits intoapache:trunkfrom
ableegoldman:InlineByteStoreTypes_Part_II
Feb 27, 2019
Merged

KAFKA-7918: Inline generic parameters Pt. II: RocksDB Bytes Store and Memory LRU Caches#6327
bbejeck merged 6 commits intoapache:trunkfrom
ableegoldman:InlineByteStoreTypes_Part_II

Conversation

@ableegoldman
Copy link
Copy Markdown
Member

@ableegoldman ableegoldman commented Feb 26, 2019

Second PR in series to inline the generic parameters of the following bytes stores:

[ Pt. I] InMemoryKeyValueStore
[x] RocksDBWindowStore
[x] RocksDBSessionStore
[x] MemoryLRUCache
[x] MemoryNavigableLRUCache
[ ] InMemoryWindowStore

Committer Checklist (excluded from commit message)

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

@ableegoldman
Copy link
Copy Markdown
Member Author

ableegoldman commented Feb 26, 2019

@guozhangwang @vvcephei @mjsax @bbejeck

ableegoldman
ableegoldman commented Feb 26, 2019
View reviewed changes
Comment thread streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBWindowStoreTest.java Outdated
Copy link
Copy Markdown
Member Author

@ableegoldman ableegoldman Feb 26, 2019

Choose a reason for hiding this comment

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

Removed this test entirely as it no longer makes sense when the RocksDB store can't and shouldn't interact with the serdes at all

@ableegoldman ableegoldman changed the title KAFKA-7918: Inline generic parameters Pt. 1: RocksDB Bytes Store and Memory LRU Caches KAFKA-7918: Inline generic parameters Pt. II: RocksDB Bytes Store and Memory LRU Caches Feb 26, 2019
@ableegoldman ableegoldman force-pushed the InlineByteStoreTypes_Part_II branch from 6335a48 to 04d8b5b Compare February 26, 2019 00:26
@mjsax mjsax added the streams label Feb 26, 2019
mjsax
mjsax reviewed Feb 26, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

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

LGTM. Just some formatting nits.

Comment thread streams/src/main/java/org/apache/kafka/streams/state/internals/MemoryNavigableLRUCache.java Outdated
Copy link
Copy Markdown
Member

@mjsax mjsax Feb 26, 2019

Choose a reason for hiding this comment

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

nit: break too long line into multipl

Comment thread streams/src/main/java/org/apache/kafka/streams/state/internals/RocksDBSessionStore.java Outdated
Copy link
Copy Markdown
Member

@mjsax mjsax Feb 26, 2019

Choose a reason for hiding this comment

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

If we only call super.init() we can remove the whole method.

Comment thread streams/src/main/java/org/apache/kafka/streams/state/internals/RocksDBSessionStore.java Outdated
Copy link
Copy Markdown
Member

@mjsax mjsax Feb 26, 2019

Choose a reason for hiding this comment

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

break too long line into multiple (one line per parameter)

Comment thread streams/src/main/java/org/apache/kafka/streams/state/internals/RocksDBSessionStore.java Outdated
Copy link
Copy Markdown
Member

@mjsax mjsax Feb 26, 2019

Choose a reason for hiding this comment

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

nit: line too long (compare previous formatting)

Comment thread streams/src/main/java/org/apache/kafka/streams/state/internals/RocksDBSessionStore.java Outdated
Copy link
Copy Markdown
Member

@mjsax mjsax Feb 26, 2019

Choose a reason for hiding this comment

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

as above

Comment thread streams/src/main/java/org/apache/kafka/streams/state/internals/RocksDBSessionStore.java Outdated
Copy link
Copy Markdown
Member

@mjsax mjsax Feb 26, 2019

Choose a reason for hiding this comment

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

as above

Comment thread streams/src/main/java/org/apache/kafka/streams/state/internals/RocksDBWindowStore.java Outdated
Copy link
Copy Markdown
Member

@mjsax mjsax Feb 26, 2019

Choose a reason for hiding this comment

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

as above (maybe also some more below -- please double check and break every line that does not display on Github at once)

Copy link
Copy Markdown
Member Author

@ableegoldman ableegoldman Feb 26, 2019

Choose a reason for hiding this comment

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

Ack

Comment thread .../src/main/java/org/apache/kafka/streams/state/internals/RocksDbWindowBytesStoreSupplier.java Outdated
Copy link
Copy Markdown
Member

@mjsax mjsax Feb 26, 2019

Choose a reason for hiding this comment

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

nit: move to next line and indent all with 4 spaces

bbejeck
bbejeck approved these changes Feb 26, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@bbejeck bbejeck left a comment

Choose a reason for hiding this comment

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

Thanks @ableegoldman. LGTM modulo comments from @mjsax.

@ableegoldman ableegoldman force-pushed the InlineByteStoreTypes_Part_II branch from bd9d10d to d001b59 Compare February 26, 2019 19:24
@ableegoldman
Copy link
Copy Markdown
Member Author

ableegoldman commented Feb 26, 2019

JDK 8 passed, JDK 11 failed on

ConsumerBounceTest.testRollingBrokerRestartsWithSmallerMaxGroupSizeConfigDisruptsBigGroup

Retest this, please

@ableegoldman
Copy link
Copy Markdown
Member Author

ableegoldman commented Feb 27, 2019

JDK 11 failed on three unrelated tests
kafka.api.PlaintextConsumerTest.testCommitMetadata
kafka.api.PlaintextEndToEndAuthorizationTest.testNoConsumeWithDescribeAclViaAssign
kafka.server.KafkaMetricReporterClusterIdTest.testClusterIdPresent

Retest this, please

@bbejeck bbejeck merged commit a8f2307 into apache:trunk Feb 27, 2019
@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented Feb 27, 2019

Merged #6327 to trunk

jarekr pushed a commit to confluentinc/kafka that referenced this pull request Apr 18, 2019
@stanislavkozlovski
Merge remote-tracking branch 'AK/trunk' into cos-trunk
d516eac 
* AK/trunk: (36 commits)
  KAFKA-7962: Avoid NPE for StickyAssignor (apache#6308)
  Address flakiness of CustomQuotaCallbackTest#testCustomQuotaCallback (apache#6330)
  KAFKA-7918: Inline generic parameters Pt. II: RocksDB Bytes Store and Memory LRU Caches (apache#6327)
  MINOR: fix parameter naming (apache#6316)
  KAFKA-7956 In ShutdownableThread, immediately complete the shutdown if the thread has not been started (apache#6218)
  MINOR: Refactor replica log dir fetching for improved logging (apache#6313)
  [TRIVIAL] Remove unused StreamsGraphNode#repartitionRequired (apache#6227)
  MINOR: Increase produce timeout to 120 seconds (apache#6326)
  KAFKA-7918: Inline generic parameters Pt. I: in-memory key-value store (apache#6293)
  MINOR: Fix line break issue in upgrade notes (apache#6320)
  KAFKA-7972: Use automatic RPC generation in SaslHandshake
  MINOR: Enable capture of full stack trace in StreamTask#process (apache#6310)
  KAFKA-7938: Fix test flakiness in DeleteConsumerGroupsTest (apache#6312)
  KAFKA-7937: Fix Flaky Test ResetConsumerGroupOffsetTest.testResetOffsetsNotExistingGroup (apache#6311)
  MINOR: Update docs to say 2.2 (apache#6315)
  KAFKA-7672 : force write checkpoint during StreamTask #suspend (apache#6115)
  KAFKA-7961; Ignore assignment for un-subscribed partitions (apache#6304)
  KAFKA-7672: Restoring tasks need to be closed upon task suspension (apache#6113)
  KAFKA-7864; validate partitions are 0-based (apache#6246)
  KAFKA-7492 : Updated javadocs for aggregate and reduce methods returning null behavior. (apache#6285)
  ...
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
@ableegoldman @pengxiaolong
KAFKA-7918: Inline generic parameters Pt. II: RocksDB Bytes Store and…
33dfbdb 
… Memory LRU Caches (apache#6327)

Second PR in series to inline the generic parameters of the following bytes stores

Reviewers: Matthias J. Sax <mjsax@apache.org>, Bill Bejeck <bbejeck@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mjsax mjsax mjsax left review comments

@bbejeck bbejeck bbejeck approved these changes

Assignees

No one assigned

Labels

streams

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ableegoldman @bbejeck @mjsax