[MINOR] Consolidate in-memory/rocksdb unit tests for window & session store#6677
Conversation
|
I'm not too attached to the name but I felt AbstractXXXStoreTest was not appropriate as that is already in use for the key-value stores, where the abstract test is extending by all kv stores (eg Caching, etc also included). At some point we should do the same for window/session stores, but the scope of this PR is just to consolidate tests for the two store flavors that share API and behavior. Call for review @bbejeck @cadonna @abbccdda @guozhangwang @vvcephei @mjsax |
ableegoldman
force-pushed
the
ConsolidateBytesStoreUnitTests
branch
from
96243e1 to
a91c79f
Compare
There was a problem hiding this comment.
Thanks for this, @ableegoldman !
Can you comment on why there are a couple of tests with different verification, depending on the implementation?
|
|
||
| @Test | ||
| public void shouldNotThrowInvalidRangeExceptionWithNegativeFromKey() { | ||
| setClassLoggerToDebug(); |
There was a problem hiding this comment.
should we reset the logger at the end of the test? I don't remember...
Co-Authored-By: ableegoldman <ableegoldman@gmail.com>
@vvcephei PR description updated with these comments |
guozhangwang
left a comment
guozhangwang
left a comment
There was a problem hiding this comment.
This is GREAT work, thanks @ableegoldman !
I made a brief pass over it and lgtm. Just one minor question.
|
|
||
| abstract String getMetricsScope(); | ||
|
|
||
| abstract void setClassLoggerToDebug(); |
There was a problem hiding this comment.
Why on WindowBytesStoreTest we need a flag whereas here we do not? Both used a segmented store internally right?
There was a problem hiding this comment.
Ah, good catch. Actually the flag should have been removed as no logging is done on the RocksDBXXXStore layer -- it's all in AbstractRocksDBSegmentedBytesStore.
Weird that shouldNotThrowInvalidRangeExceptionWithNegativeFromKey() passed anyway
|
LGTM! Merged to trunk. |
…es-14-May * AK_REPO/trunk: (24 commits) KAFKA-7321: Add a Maximum Log Compaction Lag (KIP-354) (apache#6009) KAFKA-8335; Clean empty batches when sequence numbers are reused (apache#6715) KAFKA-6455: Session Aggregation should use window-end-time as record timestamp (apache#6645) KAFKA-6521: Use timestamped stores for KTables (apache#6667) [MINOR] Consolidate in-memory/rocksdb unit tests for window & session store (apache#6677) MINOR: Include StickyAssignor in system tests (apache#5223) KAFKA-7633: Allow Kafka Connect to access internal topics without cluster ACLs (apache#5918) MINOR: Align KTableAgg and KTableReduce (apache#6712) MINOR: Fix code section formatting in TROGDOR.md (apache#6720) MINOR: Remove unnecessary OptionParser#accepts method call from PreferredReplicaLeaderElectionCommand (apache#6710) KAFKA-8352 : Fix Connect System test failure 404 Not Found (apache#6713) KAFKA-8348: Fix KafkaStreams JavaDocs (apache#6707) MINOR: Add missing option for running vagrant-up.sh with AWS to vagrant/README.md KAFKA-8344; Fix vagrant-up.sh to work with AWS properly MINOR: docs typo in '--zookeeper myhost:2181--execute' MINOR: Remove header and key/value converter config value logging (apache#6660) KAFKA-8231: Expansion of ConnectClusterState interface (apache#6584) KAFKA-8324: Add close() method to RocksDBConfigSetter (apache#6697) KAFKA-6789; Handle retriable group errors in AdminClient API (apache#5578) KAFKA-8332: Refactor ImplicitLinkedHashSet to avoid losing ordering when converting to Scala ...
… store (apache#6677) Consolidated the unit tests by having {RocksDB/InMemory}{Window/Session}StoreTest extend {Window/Session}BytesStoreTest. Besides some implementation-specific tests (eg involving segment maintenance) all tests were moved to the abstract XXXBytesStoreTest class. The test coverage now is a superset of the original test coverage for each store type. The only difference made to existing tests (besides moving them) was to switch from list-based equality comparison to set based, in order to reflect that the stores make no guarantees regarding the ordering of records returned from a range fetch. There are some implementation-specific tests that were left in the corresponding test class. The RocksDBWindowStoreTest, for example, had several tests pertaining to segments and/or the underlying filesystem. Another key difference is that the in-memory versions should delete expired records aggressively, while the RocksDB versions should only remove entirely expired segments. Reviewers: John Roesler <john@confluent.io>, Guozhang Wang <wangguoz@gmail.com>
4 participants
Consolidated the unit tests by having {RocksDB/InMemory}{Window/Session}StoreTest extend {Window/Session}BytesStoreTest. Besides some implementation-specific tests (eg involving segment maintenance) all tests were moved to the abstract XXXBytesStoreTest class. The test coverage now is a superset of the original test coverage for each store type.
The only difference made to existing tests (besides moving them) was to switch from list-based equality comparison to set based, in order to reflect that the stores make no guarantees regarding the ordering of records returned from a range fetch.
There are some implementation-specific tests that were left in the corresponding test class. The RocksDBWindowStoreTest, for example, had several tests pertaining to segments and/or the underlying filesystem. Another key difference is that the in-memory versions should delete expired records aggressively, while the RocksDB versions should only remove entirely expired segments