VSCopy: Enable to copy from all shards in either a specified keyspace or all keyspaces by yoheimuta · Pull Request #11909 · vitessio/vitess

yoheimuta · 2022-12-08T07:28:23Z

Description

This PR removes the two checks by which VTGate refuses a copy request lacking either shard or keyspace.

Let's say you have two keyspaces named ks and ks2, and each keyspace includes two shards called -80 and 80-.
Here's the table of the input and expected output:

gtid	shard	keyspace	copy targets
empty	empty	/.*	`ks.-80`, `ks.80-`, `ks2.-80`, `ks2.80-`
empty	empty	ks	`ks.-80`, `ks.80-`

Related Issue(s)

Fixes #11886

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Documentation was added or is not required

Deployment Notes

…est doesn't include keyspace and shard Signed-off-by: yoheimuta <yoheimuta@gmail.com>

…mpty gtid Signed-off-by: yoheimuta <yoheimuta@gmail.com>

Signed-off-by: yoheimuta <yoheimuta@gmail.com>

…mCopyWithoutKeyspaceShard from failing when running tests together Signed-off-by: yoheimuta <yoheimuta@gmail.com>

vitess-bot · 2022-12-08T07:28:26Z

yoheimuta · 2022-12-08T07:54:25Z

go/vt/vttest/local_cluster.go

+		if err != nil {
+			return err
+		}
+		schemaDir = tempSchemaDir


The main change in this file is the above lines.
The below is a trivial diff.

yoheimuta · 2022-12-08T07:54:35Z

go/vt/vtgate/endtoend/row_count_test.go

 	conn, err := mysql.Connect(ctx, &vtParams)
 	require.NoError(t, err)
 	defer conn.Close()
+	utils.Exec(t, conn, "use ks")


This use statement is now necessary because the number of keyspace is multiple. 3afc402

yoheimuta · 2022-12-08T07:54:42Z

go/vt/vtgate/endtoend/misc_test.go

 	require.NoError(t, err)
 	defer conn.Close()

+	// cleanup the keyspace from the topology.


A leaked "testitest" keyspace makes TestVStreamCopyWithoutKeyspaceShard/copy_from_all_keyspaces fail.
Specifically, the VStreamer API itself fails with the below error.

E1207 19:46:02.204532 47701 tablet_picker.go:179] error getting shard testitest/0: node doesn't exist: keyspaces/testitest/shards/0/Shard

To fix it, 05e5ea0 adds a new cleanup function to delete the keyspace explicitly.

Signed-off-by: yoheimuta <yoheimuta@gmail.com>

…cks either keyspace or shard Signed-off-by: yoheimuta <yoheimuta@gmail.com>

mattlord · 2022-12-13T05:44:04Z

Hi @yoheimuta,

Thank you for another PR! I just wanted to note that before we merge this, we need to catch up on the vtgate VStream RPC docs: vitessio/website#1216

We can add info there about the new ability to copy not only individual tables, but all tables in a keyspace or all tables in all keyspaces. I'll link this PR to that docs PR for now. You should be able to push directly to that PR but if not let me know and I can get you setup with the website repo.

We're not supposed to merge PRs w/o any accompanying docs but we did, and I don't want to fall further behind on that front.

(There's another tangentially related PR here too: #11771)

Thanks!

yoheimuta · 2022-12-14T09:59:06Z

Thank you for telling me! I'm checking out the docs PR.

yoheimuta · 2022-12-14T10:22:00Z

You should be able to push directly to that PR but if not let me know and I can get you setup with the website repo.

@mattlord I couldn't push directly to the PR.
Can you please look into the access rights?

(base) ❯ git push --set-upstream origin vstream_docs
ERROR: Permission to vitessio/website.git denied to yoheimuta.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

mattlord · 2022-12-28T23:20:21Z

go/vt/vtgate/vstream_manager.go

+	// that has an empty keyspace.
 	if len(vgtid.ShardGtids) == 1 && vgtid.ShardGtids[0].Keyspace == "" {
-		if vgtid.ShardGtids[0].Gtid != "current" {
-			return nil, nil, nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "for an empty keyspace, the Gtid value must be 'current': %v", vgtid)
-		}
 		keyspaces, err := vsm.toposerv.GetSrvKeyspaceNames(ctx, vsm.cell, false)
 		if err != nil {
 			return nil, nil, nil, err


I would prefer the request be more explicit as it will likely generate A LOT of load and could have a big impact on production. I think it would also be nice if the keyspace wildcard selection lined up with the table wildcard selection in binlogdata.Filter.Rule.Match, which is /.* as a valid regular expression is accepted if the string has a / prefix. So I think accepting valid regular expressions for the keyspace would be much nicer: /.*.

@mattlord It makes sense in terms of the difference in impact between "streaming" and "copy."
Accepting the keyspace wildcard sounds useful. With this feature, we don't have to accept the request with an empty keyspace and empty gtid.

Do you think the following specifications are still required?

A special request flag, for example, "EnableEmptyKeyspace" or "EnableWildcardKeyspace" in VStreamFlags.

I think only the keyspace wildcard like /.* is enough.

The valid combination of an empty keyspace and "current" gtid.

While the combination of an empty keyspace and the gtid other than "current" is invalid. It complicates the code, but not so much.

Some may think it's better to stop accepting this input for the sake of simplicity at this moment.

As such, I plan to update accepting the keyspace wildcard and preserve the previous behavior as much as possible like the below table. What do you think?

gtid keyspace valid or invalid(=noop)

"" "" invalid because of no literal match. No change.

"current" "" valid because of the compatibility. No change.

"" "/.*" valid if found because of wildcard match. New behavior.

"current" "/.*" valid if found because of wildcard match. New behavior.

"" "/ks.*/" valid if found because of wildcard match. New behavior.

"current" "/ks.*/" valid if found because of wildcard match. New behavior.

I think the explicit keyspace wildcard is enough and we don't need a new flag around that.

Now that I understand the usage of current better, I think we have to keep it in place for compatibility sake. What do you think @rohit-nayak-ps ?

Hi @rohit-nayak-ps, I'd appreciate your feedback. Let me know if anything is missing.

@yoheimuta, sorry for the delay. I am fine with the approach outlined above.

We should add a (or modify the existing) e2e test to confirm that resharding will continue to work if multiple keyspaces are being streamed. We have code (see the logic around journaler in vstream_manager), which waits for participating shards to converge, before automatically continuing to stream from the new shards. I think it should be fine, when there are multiple keyspaces, but confirming with a test will be useful.

Thank you for your confirmation!
I'm looking into e2e test too.

Hi 👋

@mattlord I implemented the keyspace wildcard selection by d83daaf and c39af7c.

@rohit-nayak-ps I added the e2e test by 71e1681 and 436568d

And I think the CI failure is caused by some unreliable tests because the failing test changes every time we run it and it passes when we run it locally.

Thanks @yoheimuta, we will review it soon. Yes, that CI failure is also happening on other PRs and I have asked the related team to look at it.

mattlord · 2022-12-28T23:21:05Z

go/vt/vtgate/vstream_manager_test.go

+			ShardGtids: []*binlogdatapb.ShardGtid{{
+				Keyspace: "TestVStream",
+			}},


These test additions are unrelated to this PR, or no? I would think we would test the new behavior, which would mean NOT specifying the shards and now not getting an error?

@mattlord I thought they were related to this PR.
Technically, I didn't add new test cases (input & output). I changed the expected output in the following existing two cases:

When the input is ShardGtids: []*binlogdatapb.ShardGtid{{}},

prev output: Returning an error

new output: Returning all shards in all keyspaces without any errors.

Moving the case code to the special-case tables because output is too big. https://github.com/vitessio/vitess/pull/11909/files#diff-3e2894943e5628aa7d63298e06c321fe82602d18693b2d5e99541fafd2c01395R1021

When the input is ShardGtids: []*binlogdatapb.ShardGtid{{Keyspace: "TestVStream"}},

prev output: Returning an error

new output: Returning all shards in the TestVStream keyspace without any errors.

https://github.com/vitessio/vitess/pull/11909/files#diff-3e2894943e5628aa7d63298e06c321fe82602d18693b2d5e99541fafd2c01395R893

Here as well I think that comments noting what we're actually testing, how, and why are helpful. The future reader won't easily see comments here on the PR. And I know that the existing code is very poorly commented, but I want to do better moving forward. 🙂

Added by 3b41d76.

go/vt/vtgate/vstream_manager_test.go

…selection Signed-off-by: yoheimuta <yoheimuta@gmail.com>

Signed-off-by: yoheimuta <yoheimuta@gmail.com>

…the streaming two keyspaces works even after reshard Signed-off-by: yoheimuta <yoheimuta@gmail.com>

Signed-off-by: yoheimuta <yoheimuta@gmail.com>

…ier to read Signed-off-by: yoheimuta <yoheimuta@gmail.com>

Signed-off-by: yoheimuta <yoheimuta@gmail.com>

yoheimuta · 2023-02-15T08:51:27Z

@mattlord Thanks for reviewing my work!
I just finished responding to your comments, so please take a look when you have a chance.

The only thing left on my to-do list for now is addressing the comment on #11909, which I'll bring up in our chat shortly.

yoheimuta · 2023-02-15T09:33:38Z

which I'll bring up in our chat shortly.

Instead, I first posted a question as a follow-up comment.
#11909 (comment)

yoheimuta · 2023-02-16T02:52:36Z

Regarding commit 8970182, a simple change to a test log message and comment unexpectedly caused 17 checks to fail. I believe these failures are unrelated to the changes in the PR, as the previous commit d7169ec only failed 1 check.

rohit-nayak-ps · 2023-02-16T06:53:25Z

Regarding commit 8970182, a simple change to a test log message and comment unexpectedly caused 17 checks to fail. I believe these failures are unrelated to the changes in the PR, as the previous commit d7169ec only failed 1 check.

The errors are due to a change in the signature of a test function, see: 8970182

You probably need to rebase to main if the tests are running locally for you.

…yspace-shard Signed-off-by: yoheimuta <yoheimuta@gmail.com>

…as introduced in the main repository Signed-off-by: yoheimuta <yoheimuta@gmail.com>

yoheimuta · 2023-02-16T09:22:04Z

@rohit-nayak-ps Thank you for your information!
I mixed up GitHub Action with CircleCI 😅.

256400f fixed the test signature.

mattlord

Thank you, @yoheimuta ! ❤️ This is a nice usability improvement.

mattlord · 2023-02-22T17:06:34Z

@yoheimuta if we merge in / rebase on main now, the tests should all be good.

yoheimuta · 2023-02-24T01:09:09Z

@mattlord Thank you for your review and helpful suggestions! I really appreciate it.

…yspace-shard Signed-off-by: yoheimuta <yoheimuta@gmail.com>

yoheimuta · 2023-02-24T01:13:31Z

Thanks for the info! I have merged the main branch.

yoheimuta · 2023-02-27T03:24:44Z

@mattlord Is there anything missing to pass the test?
I don't think the flakiness of Cluster (onlineddl_vrepl) mysql57 and Cluster (onlineddl_vrepl) are caused by this PR, though.

yoheimuta · 2023-02-27T03:24:51Z

And I'm thinking of preparing a PR for https://github.com/vitessio/website after it's been approved by two people.

mattlord · 2023-02-28T02:36:01Z

@mattlord Is there anything missing to pass the test?

I don't think so. We just need @rohit-nayak-ps to take another look when he can (PRs need 2 approvals).

I don't think the flakiness of Cluster (onlineddl_vrepl) mysql57 and Cluster (onlineddl_vrepl) are caused by this PR, though.

Those are unrelated. It's a known issue that we're discussing the proper/best solution to.

Thanks!

rohit-nayak-ps

lgtm. Nice work!

… or all keyspaces (vitessio#11909) * VSCopy: Demonstrate to fail a test case on which the vstream API request doesn't include keyspace and shard Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Copy from all shards in all keyspaces by specifying only an empty gtid Signed-off-by: yoheimuta <yoheimuta@gmail.com> * tests: Make TestRowCount stable regardless of the number of keyspaces Signed-off-by: yoheimuta <yoheimuta@gmail.com> * tests: Cleanup TestCreateAndDropDatabase correctly to stop TestVStreamCopyWithoutKeyspaceShard from failing when running tests together Signed-off-by: yoheimuta <yoheimuta@gmail.com> * tests: Tweak to fix a comment Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: fix the unit tests when the input vgtid with an empty gtid lacks either keyspace or shard Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Keyspace wildcard selection lines up with the table wildcard selection Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Tests the VCopy with multiple keyspaces and resharding Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Make TestVStreamCopyMultiKeyspaceReshard clearer to check if the streaming two keyspaces works even after reshard Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Return an invalid argument error if shards are unspecified and gtid is neither 'current' nor empty Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Add a test description about its purpose and target Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Remove duplicate literals in the test Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Retain defaultReplicas variable in the test Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Explain why we are setting Match to 'customer.*' in the test Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Remove an unused VStreamFlag for the test Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Use sentence capitalization in the test Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Verify that we didn't lose any events or get duplicates of the keyspace being reshareded in the test Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Return a value instead of a pointer because there is no need to modify the value Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Add a comment describing what TestVStreamCopyFromAllKeyspacesAndAllShards is doing and why Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Add a comment describing why we expect these specific numbers of events from VStream API Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Tweak the test case name Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Make a utility function to sort COPY_COMPLETED events in the test Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Replace the matcher with a simpler one in the test Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Move the print debug call to the FailNow section in the test Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Use require.NoError in new tests Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Use require instead of t.Fatalf in the test Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Apply the reviewer's suggestion to make the error message easier to read Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Add a comment noting what we're actually testing Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Correct the test comment and elaborate the special-case Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Tweak an error message and a comment Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Adjust to a change in the signature of a test function that was introduced in the main repository Signed-off-by: yoheimuta <yoheimuta@gmail.com> --------- Signed-off-by: yoheimuta <yoheimuta@gmail.com>

* add vtgate flag that explicitly allows vstream copy (#125) * fix fs.BoolVar Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * VSCopy: Resume the copy phase consistently from given GTID and lastpk (vitessio#11103) * VSCopy: Demonstrate to fail a test case on which the vstream API is supposed to resume the copy phase consistently Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Resume the copy phase consistently from given GTID and lastpk Signed-off-by: yoheimuta <yoheimuta@gmail.com> * Build out the unit test some more Signed-off-by: Matt Lord <mattalord@gmail.com> * Update tests for new behavior Signed-off-by: Matt Lord <mattalord@gmail.com> * Improve comments Signed-off-by: Matt Lord <mattalord@gmail.com> * Limit uvstreamer changes and update test Signed-off-by: Matt Lord <mattalord@gmail.com> * Revert uvstreamer test changes Signed-off-by: Matt Lord <mattalord@gmail.com> * Revert all uvstream changes Signed-off-by: Matt Lord <mattalord@gmail.com> * VCopy: Revert the last three commits Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VCopy: Add a new vstream type that allows picking up where we left off Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VCopy: Revert the unit test change Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VCopy: Fix the end-to-end CI test Signed-off-by: yoheimuta <yoheimuta@gmail.com> * Update logic for setting up uvstreamer based on input vgtid/tablepks. Add more catchup events to test Signed-off-by: Rohit Nayak <rohit@planetscale.com> * Refactor logic to decide if event is to be sent. Enhance unit and e2e tests. Signed-off-by: Rohit Nayak <rohit@planetscale.com> * Don't send events for tables which we can identify as ones we haven't started copy for Signed-off-by: Rohit Nayak <rohit@planetscale.com> * Minor changes after self-review Signed-off-by: Rohit Nayak <rohit@planetscale.com> * Add vstream copy resume to release notes Signed-off-by: Matt Lord <mattalord@gmail.com> * Address review comments Signed-off-by: Matt Lord <mattalord@gmail.com> Signed-off-by: yoheimuta <yoheimuta@gmail.com> Signed-off-by: Matt Lord <mattalord@gmail.com> Signed-off-by: Rohit Nayak <rohit@planetscale.com> Co-authored-by: Matt Lord <mattalord@gmail.com> Co-authored-by: Rohit Nayak <rohit@planetscale.com> * VSCopy: Send COPY_COMPLETED events when the copy operation is done (vitessio#11740) * VSCopy: Demonstrate to fail a test case on which the vstream API sends new events showing copy completed Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Send new events when the copy operation is done Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Fix typo Signed-off-by: yoheimuta <yoheimuta@gmail.com> * Initialize new map for the 'vstream * from' vtgate sql interface. Make vtadmin web protos Signed-off-by: Rohit Nayak <rohit@planetscale.com> * VSCopy: Make TestVStreamCopyBasic fail fast to avoid the end2end timeout out Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: stop sharing the 't1' table among multiple test cases running concurrently Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: refactor the function signature to be clearer Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: refactor the VEvents sorter to be simpler Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: refactor to stop the sorter from including a fully copied event Signed-off-by: yoheimuta <yoheimuta@gmail.com> Signed-off-by: yoheimuta <yoheimuta@gmail.com> Signed-off-by: Rohit Nayak <rohit@planetscale.com> Co-authored-by: Rohit Nayak <rohit@planetscale.com> * VSCopy: Enable to copy from all shards in either a specified keyspace or all keyspaces (vitessio#11909) * VSCopy: Demonstrate to fail a test case on which the vstream API request doesn't include keyspace and shard Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Copy from all shards in all keyspaces by specifying only an empty gtid Signed-off-by: yoheimuta <yoheimuta@gmail.com> * tests: Make TestRowCount stable regardless of the number of keyspaces Signed-off-by: yoheimuta <yoheimuta@gmail.com> * tests: Cleanup TestCreateAndDropDatabase correctly to stop TestVStreamCopyWithoutKeyspaceShard from failing when running tests together Signed-off-by: yoheimuta <yoheimuta@gmail.com> * tests: Tweak to fix a comment Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: fix the unit tests when the input vgtid with an empty gtid lacks either keyspace or shard Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Keyspace wildcard selection lines up with the table wildcard selection Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Tests the VCopy with multiple keyspaces and resharding Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Make TestVStreamCopyMultiKeyspaceReshard clearer to check if the streaming two keyspaces works even after reshard Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Return an invalid argument error if shards are unspecified and gtid is neither 'current' nor empty Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Add a test description about its purpose and target Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Remove duplicate literals in the test Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Retain defaultReplicas variable in the test Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Explain why we are setting Match to 'customer.*' in the test Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Remove an unused VStreamFlag for the test Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Use sentence capitalization in the test Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Verify that we didn't lose any events or get duplicates of the keyspace being reshareded in the test Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Return a value instead of a pointer because there is no need to modify the value Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Add a comment describing what TestVStreamCopyFromAllKeyspacesAndAllShards is doing and why Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Add a comment describing why we expect these specific numbers of events from VStream API Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Tweak the test case name Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Make a utility function to sort COPY_COMPLETED events in the test Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Replace the matcher with a simpler one in the test Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Move the print debug call to the FailNow section in the test Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Use require.NoError in new tests Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Use require instead of t.Fatalf in the test Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Apply the reviewer's suggestion to make the error message easier to read Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Add a comment noting what we're actually testing Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Correct the test comment and elaborate the special-case Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Tweak an error message and a comment Signed-off-by: yoheimuta <yoheimuta@gmail.com> * VSCopy: Adjust to a change in the signature of a test function that was introduced in the main repository Signed-off-by: yoheimuta <yoheimuta@gmail.com> --------- Signed-off-by: yoheimuta <yoheimuta@gmail.com> * attempt unit test fix Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * update test error expected Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> Signed-off-by: yoheimuta <yoheimuta@gmail.com> Signed-off-by: Matt Lord <mattalord@gmail.com> Signed-off-by: Rohit Nayak <rohit@planetscale.com> Co-authored-by: pbibra <pbibra@slack-corp.com> Co-authored-by: yohei yoshimuta <yoheimuta@gmail.com> Co-authored-by: Matt Lord <mattalord@gmail.com> Co-authored-by: Rohit Nayak <rohit@planetscale.com>

yoheimuta added 4 commits December 7, 2022 22:21

VSCopy: Demonstrate to fail a test case on which the vstream API requ…

077136b

…est doesn't include keyspace and shard Signed-off-by: yoheimuta <yoheimuta@gmail.com>

VSCopy: Copy from all shards in all keyspaces by specifying only an e…

2f7adbc

…mpty gtid Signed-off-by: yoheimuta <yoheimuta@gmail.com>

tests: Make TestRowCount stable regardless of the number of keyspaces

3afc402

Signed-off-by: yoheimuta <yoheimuta@gmail.com>

tests: Cleanup TestCreateAndDropDatabase correctly to stop TestVStrea…

05e5ea0

…mCopyWithoutKeyspaceShard from failing when running tests together Signed-off-by: yoheimuta <yoheimuta@gmail.com>

yoheimuta requested review from GuptaManan100, deepthi, frouioui, harshit-gangal and systay as code owners December 8, 2022 07:28

frouioui added Type: Feature Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Query Serving Component: VReplication labels Dec 8, 2022

yoheimuta commented Dec 8, 2022

View reviewed changes

yoheimuta added 2 commits December 8, 2022 17:10

tests: Tweak to fix a comment

013b2f0

Signed-off-by: yoheimuta <yoheimuta@gmail.com>

VSCopy: fix the unit tests when the input vgtid with an empty gtid la…

fdb2b68

…cks either keyspace or shard Signed-off-by: yoheimuta <yoheimuta@gmail.com>

mattlord requested review from mattlord and rohit-nayak-ps December 13, 2022 05:31

mattlord mentioned this pull request Dec 13, 2022

Document VStream vitessio/website#1216

Merged

mattlord requested changes Dec 28, 2022

View reviewed changes

deepthi removed their request for review January 4, 2023 20:02

yoheimuta added 3 commits February 7, 2023 19:19

VSCopy: Keyspace wildcard selection lines up with the table wildcard …

d83daaf

…selection Signed-off-by: yoheimuta <yoheimuta@gmail.com>

VSCopy: Tests the VCopy with multiple keyspaces and resharding

71e1681

Signed-off-by: yoheimuta <yoheimuta@gmail.com>

VSCopy: Make TestVStreamCopyMultiKeyspaceReshard clearer to check if …

436568d

…the streaming two keyspaces works even after reshard Signed-off-by: yoheimuta <yoheimuta@gmail.com>

yoheimuta added 8 commits February 14, 2023 16:02

VSCopy: Replace the matcher with a simpler one in the test

cd119d4

Signed-off-by: yoheimuta <yoheimuta@gmail.com>

VSCopy: Move the print debug call to the FailNow section in the test

f7e480f

Signed-off-by: yoheimuta <yoheimuta@gmail.com>

VSCopy: Use require.NoError in new tests

e8f0e60

Signed-off-by: yoheimuta <yoheimuta@gmail.com>

VSCopy: Use require instead of t.Fatalf in the test

ba8c231

Signed-off-by: yoheimuta <yoheimuta@gmail.com>

VSCopy: Apply the reviewer's suggestion to make the error message eas…

d730563

…ier to read Signed-off-by: yoheimuta <yoheimuta@gmail.com>

VSCopy: Add a comment noting what we're actually testing

3b41d76

Signed-off-by: yoheimuta <yoheimuta@gmail.com>

VSCopy: Correct the test comment and elaborate the special-case

d7169ec

Signed-off-by: yoheimuta <yoheimuta@gmail.com>

VSCopy: Tweak an error message and a comment

8970182

Signed-off-by: yoheimuta <yoheimuta@gmail.com>

yoheimuta added 2 commits February 16, 2023 17:43

Merge remote-tracking branch 'origin/main' into vscopy-unspecified-ke…

38eb575

…yspace-shard Signed-off-by: yoheimuta <yoheimuta@gmail.com>

VSCopy: Adjust to a change in the signature of a test function that w…

256400f

…as introduced in the main repository Signed-off-by: yoheimuta <yoheimuta@gmail.com>

mattlord approved these changes Feb 22, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into vscopy-unspecified-ke…

94beeac

…yspace-shard Signed-off-by: yoheimuta <yoheimuta@gmail.com>

rohit-nayak-ps approved these changes Mar 14, 2023

View reviewed changes

rohit-nayak-ps merged commit 5ba5f04 into vitessio:main Mar 14, 2023

pbibra mentioned this pull request Sep 14, 2023

apply patch 11909 slackhq/vitess#131

Merged

timvaillancourt mentioned this pull request May 27, 2024

slack-15.0: VStreams fix backports/patches, pt. 1 slackhq/vitess#373

Merged

4 tasks

gtid	keyspace	valid or invalid(=noop)
""	""	invalid because of no literal match. No change.
"current"	""	valid because of the compatibility. No change.
""	"/.*"	valid if found because of wildcard match. New behavior.
"current"	"/.*"	valid if found because of wildcard match. New behavior.
""	"/ks.*/"	valid if found because of wildcard match. New behavior.
"current"	"/ks.*/"	valid if found because of wildcard match. New behavior.

Conversation

yoheimuta commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

Checklist

Deployment Notes

Uh oh!

vitess-bot bot commented Dec 8, 2022

Review Checklist

General

If a new flag is being introduced:

If a workflow is added or modified:

Bug fixes

Non-trivial changes

New/Existing features

Backward compatibility

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattlord commented Dec 13, 2022

Uh oh!

yoheimuta commented Dec 14, 2022

Uh oh!

yoheimuta commented Dec 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattlord Feb 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yoheimuta commented Feb 15, 2023

Uh oh!

yoheimuta commented Feb 15, 2023

Uh oh!

yoheimuta commented Feb 16, 2023

Uh oh!

rohit-nayak-ps commented Feb 16, 2023

Uh oh!

yoheimuta commented Feb 16, 2023

Uh oh!

mattlord left a comment

Choose a reason for hiding this comment

Uh oh!

mattlord commented Feb 22, 2023

Uh oh!

yoheimuta commented Feb 24, 2023

Uh oh!

yoheimuta commented Feb 24, 2023

Uh oh!

yoheimuta commented Feb 27, 2023

Uh oh!

yoheimuta commented Feb 27, 2023

Uh oh!

yoheimuta commented Dec 8, 2022 •

edited

Loading

mattlord Feb 11, 2023 •

edited

Loading