changefeedccl: Various changes and fixes #81105
Conversation
Native encoder is no longer needed since streaming replication no lnoger users changefeed machinery. Release Notes: None
Pure code move refactoring to move topic related code to topic file. Release Notes: None
Remove a 10 second sleep in `TestChangefeedEndTime`. Instead of sleeping for 10 seconds, introducing a testing knob that indicates the end time has been reached. Release Note: None
There is no reason to restrict `TestAlterChangefeedPersistSinkURI` and `TestAlterChangefeedChangeSinkTypeError` tests to only run if env variables for AWS access are set. Release Notes: None
|
This change is |
| require.False(t, seenMoreMessages) | ||
| }() | ||
|
|
||
| close(endTimeReached) |
There was a problem hiding this comment.
I feel like we're no longer testing enough of the endTime implementation, since now we're skipping over all the code around setting the scanboundary at the right timestamp and reacting appropriately. We're only verifying that the changefeed ends with Success if end_time was set since the knob does all of the initial work of stopping the feed. We could miss bugs where we accidentally emit extra messages that were emitted after the endTime.
It'd also be nice to sneak in a schema change after creating the changefeed but before the endTime just to introduce another scan boundary that shouldn't be invalidated by the current scanboundary-based endtime code.
There was a problem hiding this comment.
Well, perhaps it's not as thorough; but I'm not concerned. First, sleep, particularly sleep for 10 seconds is really not great. Secondly, we still do set the boundary -- it's just triggering scan boundary at different time than it would otherwise.
We could miss bugs where we accidentally emit extra messages that were emitted after the endTime.
Perhaps; but also consider the fact that the existing check for those messages is racy -- it happens to work because
of how our fake test feed implementations work. So, I don't really feel it's loosing that much coverage.
It'd also be nice to sneak in a schema change after creating the changefeed but before the endTime just to introduce another scan boundary that shouldn't be invalidated by the current scanboundary-based endtime code.
Sure, we can improve this test... I'm just trying to fix what's broken right now - namely the 10 second sleep.
There was a problem hiding this comment.
I do still feel uncomfortable about lowering the test coverage even though it works via our testfeed details + sleep, but not a strong opinion that'd block merge
|
@samiskin let me know if you have more comments. |
| require.False(t, seenMoreMessages) | ||
| }() | ||
|
|
||
| close(endTimeReached) |
There was a problem hiding this comment.
I do still feel uncomfortable about lowering the test coverage even though it works via our testfeed details + sleep, but not a strong opinion that'd block merge
|
tftr. |
|
Build succeeded: |
See commits for details.
Release Notes: None