MINOR: simplify window-byte-store-supplier#21829
Conversation
mjsax
force-pushed
the
minor-simplify-window-store-supplier
branch
from
35d3cf9 to
103b650
Compare
mjsax
force-pushed
the
minor-simplify-window-store-supplier
branch
from
103b650 to
d772288
Compare
| if (segmentInterval < 1L) { | ||
| throw new IllegalArgumentException("segmentInterval cannot be zero or negative"); | ||
| } | ||
| if (windowSize > retentionPeriod) { |
There was a problem hiding this comment.
windowSize > retentionPeriod validation was dropped.
Note that RocksDbIndexedTimeOrderedWindowBytesStoreSupplier.create() still keeps this exact validation (line 66-69), so it seems like an unintentional omission.
There was a problem hiding this comment.
Ups. Deleted too much -- only wanted to delete the segmentInterval check.
| final Duration windowSize, | ||
| final boolean retainDuplicates, | ||
| final boolean timestampedStore) { | ||
| final RocksDbWindowBytesStoreSupplier.WindowStoreTypes timestampedStoreType) { |
aliehsaeedii
left a comment
aliehsaeedii
left a comment
There was a problem hiding this comment.
Thanks @mjsax. LGTM, but I see some changes in indentation which I don't know if they were intentional.
| throw new IllegalArgumentException("windowSize cannot be negative"); | ||
| } | ||
| if (defaultSegmentInterval < 1L) { | ||
| throw new IllegalArgumentException("segmentInterval cannot be zero or negative"); |
There was a problem hiding this comment.
This check can never execute as we compute defaultSegmentInterval above ourselves.
| throw new IllegalArgumentException("windowSize cannot be negative"); | ||
| } | ||
| if (segmentInterval < 1L) { | ||
| throw new IllegalArgumentException("segmentInterval cannot be zero or negative"); |
There was a problem hiding this comment.
We compute segmentInterval ourselves -- cannot be smaller than 1L.
| if (segmentInterval < 1L) { | ||
| throw new IllegalArgumentException("segmentInterval cannot be zero or negative"); | ||
| } | ||
| if (windowSize > retentionPeriod) { |
There was a problem hiding this comment.
Ups. Deleted too much -- only wanted to delete the segmentInterval check.
DL1231
left a comment
DL1231
left a comment
There was a problem hiding this comment.
Thanks for the patch, LGTM.
4 participants
This PR removes an unnecessary "duplicated" constructor for
RocksDbWindowBytesStoreSupplier, and unifies the caller stack to re-use
more shared code.
Also make internal contractors non-public for
RocksDbIndexedTimeOrderedWindowBytesStoreSupplier.
Reviewers: Lan Ding isDing_L@163.com, Alieh Saeedi
asaeedi@confluent.io, TengYao Chi frankvicky@apache.org