-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
VReplication: Support for BETWEEN
/NOT BETWEEN
filter in VStream
#17721
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Noble Mittal <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17721 +/- ##
==========================================
- Coverage 67.97% 67.96% -0.01%
==========================================
Files 1586 1586
Lines 255195 255305 +110
==========================================
+ Hits 173468 173522 +54
- Misses 81727 81783 +56 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Noble Mittal <[email protected]>
Tested this locally using the example local cluster. |
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really good. Nice work on this so far!
Can you please look at the TestVStreamPushdownFilters
endtoend test in go/test/endtoend/vreplication/vstream_test.go
? We should add to that (additional test cases) or mimic it. This way we have an end to end test that covers both the copy phase (rowstreamer filtering) and the running phase (vstreamer filtering).
Please let me know if I can be of any help!
Signed-off-by: Noble Mittal <[email protected]>
@beingnoble03 let's update the website docs for this: https://vitess.io/docs/22.0/reference/vreplication/materialize/ (https://github.com/vitessio/website). Also check if we need to update that doc with the newly supported filter predicates since the docs were last updated. |
Signed-off-by: Noble Mittal <[email protected]>
@@ -1127,7 +1127,7 @@ func TestVStreamPushdownFilters(t *testing.T) { | |||
require.NoError(t, err) | |||
|
|||
// Coordinate go-routines. | |||
streamCtx, streamCancel := context.WithTimeout(context.Background(), 1*time.Minute) | |||
streamCtx, streamCancel := context.WithTimeout(context.Background(), 10*time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattlord This test was taking around 90 seconds to complete. Can you please let me know if there's a specific reason the timeout was set to 1 min? Is it okay to decrease it? Setting it to 10 sec still gets around 2 copy phase rows and ~70 running phase rows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an end to end test. 90 seconds is quite fast for an end to end test. At the same time... cutting it in half (or potentially even more) would be OK.
Description
This PR adds support for
BETWEEN
filter in VStream.Related Issue(s)
Checklist
Deployment Notes