MINOR: Remove 1 minute minimum segment interval #5323
Conversation
| throw new IllegalArgumentException("windowSize cannot be negative"); | ||
| } | ||
| if (segmentInterval < 60_000) { | ||
| throw new IllegalArgumentException("segmentInterval must be at least one minute"); | ||
| if (segmentInterval < 0L) { |
There was a problem hiding this comment.
I set the min to 0, following the other two bounds, but wouldn't 0 cause problems in practice? Should we instead set all three minima to 1?
There was a problem hiding this comment.
I'd suggest setting the min to 0 other than 1, because interval maybe used as denominator in some use cases?
| @@ -38,6 +38,7 @@ public void shouldThrowIfPersistentKeyValueStoreStoreNameIsNull() { | |||
|
|
|||
| @Test(expected = NullPointerException.class) | |||
| public void shouldThrowIfIMemoryKeyValueStoreStoreNameIsNull() { | |||
| //noinspection ResultOfMethodCallIgnored | |||
There was a problem hiding this comment.
I've also included some general cleanup of warnings.
| public class CachingSessionStoreTest { | ||
|
|
||
| private static final int MAX_CACHE_SIZE_BYTES = 600; | ||
| private InternalMockProcessorContext context; | ||
| private static final Long DEFAULT_TIMESTAMP = 10L; | ||
| private static final long SEGMENT_INTERVAL = 100L; |
There was a problem hiding this comment.
I searched through the code base to find all the spots where we set the segment interval to 1 minute just because it was the min. I've set them all lower (typically to 100ms, because it's a nice round number) mostly to avoid implying that there's anything special about 60s anymore.
| final Windowed<Bytes> a3 = new Windowed<>(keyA, new SessionWindow(120_000, 120_000)); | ||
| final Windowed<Bytes> a1 = new Windowed<>(keyA, new SessionWindow(SEGMENT_INTERVAL * 0, SEGMENT_INTERVAL * 0)); | ||
| final Windowed<Bytes> a2 = new Windowed<>(keyA, new SessionWindow(SEGMENT_INTERVAL * 1, SEGMENT_INTERVAL * 1)); | ||
| final Windowed<Bytes> a3 = new Windowed<>(keyA, new SessionWindow(SEGMENT_INTERVAL * 2, SEGMENT_INTERVAL * 2)); |
There was a problem hiding this comment.
In all the places where the test logic actually depends on the segment interval, I've expanded the expressions to include it. Theoretically, we should be able to plug in any number for the segment interval, and the tests should still pass.
|
Here's the follow-up I promised in #5257 to de-magicify the minimum segment interval. -John |
There was a problem hiding this comment.
@vvcephei is this PR complete already? The only non-test class included is Stores, but we have other classes to be considered, like minimumSegmentInterval in Windows, right?
|
Hey @guozhangwang, Sorry about the slow reply. I left that occurrence on purpose, as well as the one in RocksDbSessionBytesStoreSupplier. The reasoning is that this change only lifts the restriction. It shouldn't change the actual segment size for any existing application. If upgrading to 2.1 (no code change) caused a change in segment size, we would: Stores is the only thing that changed because I only wanted to lift the restriction, and Stores was the only location that actually enforced the restriction. |
|
Also, about the minimum segmentInterval being 0 or 1 in stores, you were right: segmentInterval is used in the denominator, to determine the segmentId. Maybe I misunderstood, but this seems like another good reason not to allow 0. Otherwise, it would throw a mysterious arithmetic exception instead of a nice(r) IllegalArgumentException. Plus, it's evidence that no application could use a segmentInterval of 0. I've updated the PR to set it to 1. |
|
@mjsax @guozhangwang : just renewing my request for reviews, since I let this PR rot for a week. |
| oldSegment.createNewFile(); | ||
| } | ||
|
|
||
| segments.openExisting(context); | ||
|
|
||
| for (int segmentId = 0; segmentId < NUM_SEGMENTS; ++segmentId) { | ||
| final File newSegment = new File(storeDirectoryPath + File.separator + storeName + "." + segmentId * (retentionPeriod / (NUM_SEGMENTS - 1))); | ||
| final File newSegment = new File(storeDirectoryPath + File.separator + segments.segmentName(segmentId)); |
There was a problem hiding this comment.
We should assemble the new format manually -- otherwise, if segmentName() is change accidentally, we would not notice.
* new minimum is 1ms * refactor tests to use smaller segment sizes as well
|
@guozhangwang @mjsax : I believe I've addressed all your comments. |
Committer Checklist (excluded from commit message)