Use long windows when catching up #67
Conversation
| public static TaskState createInitialFor(GenerationId generation, Timestamp now, | ||
| long confidenceWindowSizeMs, long queryTimeWindowSizeMs) { | ||
| // Start reading at generation start: | ||
| Timestamp windowStart = generation.getGenerationStart(); |
There was a problem hiding this comment.
What was the conclusion of your tests @avelanarius? Does it make sense to take max(generation.getGenerationStart(), now - CDC ttl)?
There was a problem hiding this comment.
Yes, it made sense. The test was the following: create a table with small TTL (3 seconds), insert 10^6 rows, then do two types of selects: select of small window and select of a large window. Both of those queries returned no rows (3 second TTL...), but a select of small window was faster. Sometimes, select of a large window even caused timeouts (even though no rows were returned). We hypothesized that this is due to replicas sending "tombstones" to coordinator.
| // queryTimeWindowSizeMs large (the consumer might need to wait a bit | ||
| // for the window to be ready for reading). | ||
| Timestamp windowEnd = now.plus(-confidenceWindowSizeMs, ChronoUnit.MILLIS); | ||
| if (windowEnd.compareTo(windowStart) < 0) { |
There was a problem hiding this comment.
Is this enough? Wouldn't we want to adjust windowEnd also when windowEnd - windowStart < queryTimeWindowSizeMs?
There was a problem hiding this comment.
Maybe, but this is only an initial window so it doesn't really matter.
There was a problem hiding this comment.
I could add this at the cost of additional complexity.
|
|
||
| return this; | ||
| // Trim the start of the window with minimumWindowStart. | ||
| Timestamp newWindowStart = windowStart; |
There was a problem hiding this comment.
Why wasn't this needed before and now is? Or was it and it was a bug we weren't trimming the start?
There was a problem hiding this comment.
Previously it only handled one case:
---- WINDOW --- | TTL
->
---- WINDOW ---
Now it also handles this case:
---- WINDOW ---
| TTL
->
- WINDOW -
| TTL
Previously it didn't really matter to do the second type of trimming, because the windows were very small (default 30 seconds).
Changes the strategy the CDC library uses to generate windows to read the CDC log. Previously, when a library was started, it would read queryTimeWindowSize large windows starting from |now - ttl|. The issue with that approach is that it requires a large number of windows to catch up, for example: 24 hours / 30 seconds * 256 * 8 = 5898240 for a 8-shard Scylla server. A new approach just creates a single large window from |now - ttl| to |now - confidence window|.
avelanarius
force-pushed
the
long-windows
branch
from
1b7f0ba to
ec66dab
Compare
|
v2: fixed flaky unit test. (off-by-one error) |
|
Any news here? I'm pretty interested in this PR |
dkropachev
force-pushed
the
master
branch
4 times, most recently
from
012c0c8 to
c89661c
Compare
dkropachev
force-pushed
the
master
branch
16 times, most recently
from
396912a to
78c0d0b
Compare
dkropachev
force-pushed
the
master
branch
13 times, most recently
from
3c4bf55 to
3318b32
Compare
dkropachev
force-pushed
the
master
branch
8 times, most recently
from
5031da9 to
b5730b8
Compare
3 participants
Changes the strategy the CDC library uses to generate windows to read the CDC log. Previously, when a library was started, it would read queryTimeWindowSize large windows starting from |now - ttl|. The issue with that approach is that it requires a large number of windows to catch up, for example: 24 hours / 30 seconds * 256 * 8 = 5898240 for a 8-shard Scylla server.
A new approach just creates a single large window from |now - ttl| to |now - confidence window|.