Skip to content

VSCopy: Send COPY_COMPLETED events when the copy operation is done#11740

Merged
mattlord merged 9 commits intovitessio:mainfrom
yoheimuta:send-copy-done-event
Nov 30, 2022
Merged

VSCopy: Send COPY_COMPLETED events when the copy operation is done#11740
mattlord merged 9 commits intovitessio:mainfrom
yoheimuta:send-copy-done-event

Conversation

@yoheimuta
Copy link
Contributor

@yoheimuta yoheimuta commented Nov 16, 2022 •
edited
Loading

Description

The COPY_COMPLETED event is sent every time either condition is met:

  • The copy of data in a shard is done. The event has a keyspace and shard value.
  • The copy of data in all the target shards is done. The event has no keyspace and shard value.

In most cases (e.g., my use case), only the event without values is enough for error handling.
In some cases, the event corresponding to a single shard is helpful to handle it in a customized way.

Related Issue(s)

#11764

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

@yoheimuta
VSCopy: Demonstrate to fail a test case on which the vstream API send…
e0f36b2 
…s new events showing copy completed

Signed-off-by: yoheimuta <yoheimuta@gmail.com>
@yoheimuta
VSCopy: Send new events when the copy operation is done
a91277a 
Signed-off-by: yoheimuta <yoheimuta@gmail.com>
@yoheimuta
VSCopy: Fix typo
5a6440b 
Signed-off-by: yoheimuta <yoheimuta@gmail.com>
@vitess-bot
Copy link
Contributor

vitess-bot bot commented Nov 16, 2022 •
edited by mattlord
Loading

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • If this is a change that users need to know about, please apply the release notes (needs details) label so that merging is blocked unless the summary release notes document is included.

If a new flag is being introduced:

  • Is it really necessary to add this flag?
  • Flag names should be clear and intuitive (as far as possible)
  • Help text should be descriptive.
  • Flag names should use dashes (-) as word separators rather than underscores (_).

If a workflow is added or modified:

  • Each item in Jobs should be named in order to mark it as required.
  • If the workflow should be required, the maintainer team should be notified.

Bug fixes

  • There should be at least one unit or end-to-end test.
  • The Pull Request description should include a link to an issue that describes the bug.

Non-trivial changes

  • There should be some code comments as to why things are implemented the way they are.

New/Existing features

  • Should be documented, either by modifying the existing documentation or creating new documentation.
  • New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • vtctl command output order should be stable and awk-able.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from VTop, if used there.

yoheimuta
yoheimuta commented Nov 16, 2022
View reviewed changes
go/vt/vtgate/endtoend/vstream_test.go
`type:COPY_COMPLETED keyspace:"ks" shard:"80-"`,
`type:COPY_COMPLETED keyspace:"ks" shard:"-80"`,
`type:COPY_COMPLETED`,
}
Copy link
Contributor Author

@yoheimuta yoheimuta Nov 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this PR change, the above three events are added to the expected events.

yoheimuta
yoheimuta commented Nov 16, 2022
View reviewed changes
go/vt/vtgate/vstream_manager.go Outdated
case binlogdatapb.VEventType_COPY_COMPLETED:
sendevents = append(sendevents, event)
if fullyCopied := vs.generateFullyCopyCompleted(ctx, sgtid, event); fullyCopied != nil {
sendevents = append(sendevents, fullyCopied)
Copy link
Contributor Author

@yoheimuta yoheimuta Nov 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only this addition is different from the case of, for example, OTHER.
In the case of COPY_COMPLETED, it's possible to generate and send an extra event.

yoheimuta
yoheimuta commented Nov 16, 2022
View reviewed changes
go/vt/vtgate/vstream_manager.go
// This new event represents the completion of all the copy operations.
func (vs *vstream) generateFullyCopyCompleted(ctx context.Context, sgtid *binlogdatapb.ShardGtid, event *binlogdatapb.VEvent) *binlogdatapb.VEvent {
vs.mu.Lock()
defer vs.mu.Unlock()
Copy link
Contributor Author

@yoheimuta yoheimuta Nov 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function generates an additional event while holding a lock for vs.vgtid.ShardGtids.
The same lock also protects the concurrent access to vs.copyCompletedShard.

yoheimuta
yoheimuta commented Nov 16, 2022
View reviewed changes
go/vt/vtgate/vstream_manager.go
}
}
return &binlogdatapb.VEvent{
Type: binlogdatapb.VEventType_COPY_COMPLETED,
Copy link
Contributor Author

@yoheimuta yoheimuta Nov 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This event has neither keyspace nor shard.
It represents the completion of data copy wherever the data is.

@yoheimuta
Copy link
Contributor Author

yoheimuta commented Nov 16, 2022

@mattlord @rohit-nayak-ps I made the changes I referred to at the slack thread.
Can you please review this PR when you have time?
If the implementation design is OK and the failed tests are not just flaky, I'm willing to dig into the failed checks.

@rohit-nayak-ps rohit-nayak-ps added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: VReplication labels Nov 16, 2022
@mattlord
Copy link
Member

mattlord commented Nov 16, 2022

@mattlord @rohit-nayak-ps I made the changes I referred to at the slack thread.
Can you please review this PR when you have time?
If the implementation design is OK and the failed tests are not just flaky, I'm willing to dig into the failed checks.

Thanks, @yoheimuta ! ❤️ These failures do seem legit, so can you look into those in the meantime? I will review this ASAP but no reason to wait on that front. We can discuss on Slack too if you need any help.

@rohit-nayak-ps
Initialize new map for the 'vstream * from' vtgate sql interface. Mak…
8912112 
…e vtadmin web protos

Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@rohit-nayak-ps
Copy link
Member

rohit-nayak-ps commented Nov 16, 2022

@mattlord @rohit-nayak-ps I made the changes I referred to at the slack thread. Can you please review this PR when you have time? If the implementation design is OK and the failed tests are not just flaky, I'm willing to dig into the failed checks.

I fixed the failing TestVReplicationCellAlias. The issue was related to this "hidden" experimental feature: #9392.

We also need to run make vtadmin_web_proto separately today, when a proto changes, in addition to make proto. (We plan to remove the need for the two steps soon ...). I did that. Let's see if any failing tests remain.

If there is not already a related issue for this, can you create one and link the PR to it (in Related Issues). Maybe you can just move the description of the problem statement that you have here into the issue ... To help discovery for open source users, we are trying to enforce that all PRs require an issue and the problem description and use case is in the issue and the resolution in the PR. Thx!

@rohit-nayak-ps
Copy link
Member

rohit-nayak-ps commented Nov 16, 2022

@mattlord @rohit-nayak-ps I made the changes I referred to at [the slack thread]

I had a quick look at your changes and they look pretty good!

@yoheimuta
Copy link
Contributor Author

yoheimuta commented Nov 17, 2022

Thank you for your advices and fixes!

If there is not already a related issue for this, can you create one and link the PR to it (in Related Issues).

It makes sense.
I created a new issue #11764 and linked the PR to it. It looks more organized 👍

@yoheimuta
VSCopy: Make TestVStreamCopyBasic fail fast to avoid the end2end time…
5825b27 
…out out

Signed-off-by: yoheimuta <yoheimuta@gmail.com>
@yoheimuta
Copy link
Contributor Author

yoheimuta commented Nov 17, 2022

I'm fixing the failure of End-to-End Test in a bit.

@yoheimuta
VSCopy: stop sharing the 't1' table among multiple test cases running…
9e4b5d8 
… concurrently

Signed-off-by: yoheimuta <yoheimuta@gmail.com>
yoheimuta
yoheimuta commented Nov 17, 2022
View reviewed changes
go/vt/vtgate/endtoend/vstream_test.go
t.Logf("TestVStreamCopyBasic was successful")
return
} else if numExpectedEvents < len(evs) {
t.Fatalf("len(events)=%v are not expected\n", len(evs))
Copy link
Contributor Author

@yoheimuta yoheimuta Nov 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, once len(evs) gets greater than numExpectedEvents, End-to-End Test job failed due to the timeout while showing no specific errors. With this error message, it's hard to narrow down the cause.

ok  	vitess.io/vitess/go/vt/vtctl/grpcvtctldserver/endtoend	0.130s
Error: The action has timed out.

5825b27 makes the same test fail faster and shows more straightforward error messages.

--- FAIL: TestVStreamCopyBasic (0.02s)
    vstream_test.go:244: len(events)=24 are not expected
...
FAIL	vitess.io/vitess/go/vt/vtgate/endtoend	77.838s

yoheimuta
yoheimuta commented Nov 17, 2022
View reviewed changes
go/vt/vtgate/endtoend/vstream_test.go
defer closeConnections()

_, err := conn.ExecuteFetch("insert into t1(id1,id2) values(1,1), (2,2), (3,3), (4,4), (5,5), (6,6), (7,7), (8,8)", 1, false)
_, err := conn.ExecuteFetch("insert into t1_copy_basic(id1,id2) values(1,1), (2,2), (3,3), (4,4), (5,5), (6,6), (7,7), (8,8)", 1, false)
Copy link
Contributor Author

@yoheimuta yoheimuta Nov 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To pass the End-to-End Test, I prepared the exclusive table named t1_copy_basic. 9e4b5d8

The problem sharing t1 table with the other test cases (e.g. TestConsistentLookupMultiInsertIgnore) running concurrently is not introduced by this PR. Although the number of the received events matches by chance, content of the events vary depending on the concurrent tests in the first place.

(base) ❯ git branch | grep main
* main

# The events are expected.
(base) ❯ go test -v -count=1 -run "^TestVStreamCopyBasic$" vitess.io/vitess/go/vt/vtgate/endtoend --alsologtostderr
...
===START===
Event 0; type:BEGIN keyspace:"ks" shard:"-80"
Event 1; type:FIELD field_event:{table_name:"ks.t1" fields:{name:"id1" type:INT64 table:"t1" org_table:"t1" database:"vt_ks_-80" org_name:"id1" column_length:20 charset:63 flags:53251} fields:{name:"id2" type:INT64 table:"t1" org_table:"t1" database:"vt_ks_-80" org_name:"id2" column_length:20 charset:63 flags:32768} keyspace:"ks" shard:"-80"} keyspace:"ks" shard:"-80"
Event 2; type:VGTID vgtid:{shard_gtids:{keyspace:"ks" shard:"-80" gtid:"MySQL56/82178ac0-6666-11ed-b225-e84ab9c3a7f8:1-75" table_p_ks:{table_name:"t1" lastpk:{fields:{name:"id1" type:INT32} rows:{lengths:1 values:"4"}}}} shard_gtids:{keyspace:"ks" shard:"80-" table_p_ks:{table_name:"t1" lastpk:{fields:{name:"id1" type:INT32} rows:{lengths:1 values:"4"}}}}} keyspace:"ks" shard:"-80"
Event 3; type:ROW row_event:{table_name:"ks.t1" row_changes:{after:{lengths:1 lengths:1 values:"55"}} keyspace:"ks" shard:"-80"} keyspace:"ks" shard:"-80"
Event 4; type:VGTID vgtid:{shard_gtids:{keyspace:"ks" shard:"-80" gtid:"MySQL56/82178ac0-6666-11ed-b225-e84ab9c3a7f8:1-75" table_p_ks:{table_name:"t1" lastpk:{rows:{lengths:1 values:"5"}}}} shard_gtids:{keyspace:"ks" shard:"80-" table_p_ks:{table_name:"t1" lastpk:{fields:{name:"id1" type:INT32} rows:{lengths:1 values:"4"}}}}} keyspace:"ks" shard:"-80"
Event 5; type:COMMIT keyspace:"ks" shard:"-80"
Event 6; type:BEGIN keyspace:"ks" shard:"80-"
Event 7; type:FIELD field_event:{table_name:"ks.t1" fields:{name:"id1" type:INT64 table:"t1" org_table:"t1" database:"vt_ks_80-" org_name:"id1" column_length:20 charset:63 flags:53251} fields:{name:"id2" type:INT64 table:"t1" org_table:"t1" database:"vt_ks_80-" org_name:"id2" column_length:20 charset:63 flags:32768} keyspace:"ks" shard:"80-"} keyspace:"ks" shard:"80-"
Event 8; type:VGTID vgtid:{shard_gtids:{keyspace:"ks" shard:"-80" gtid:"MySQL56/82178ac0-6666-11ed-b225-e84ab9c3a7f8:1-75" table_p_ks:{table_name:"t1" lastpk:{rows:{lengths:1 values:"5"}}}} shard_gtids:{keyspace:"ks" shard:"80-" gtid:"MySQL56/82178ac0-6666-11ed-b225-e84ab9c3a7f8:1-75" table_p_ks:{table_name:"t1" lastpk:{fields:{name:"id1" type:INT32} rows:{lengths:1 values:"4"}}}}} keyspace:"ks" shard:"80-"
Event 9; type:ROW row_event:{table_name:"ks.t1" row_changes:{after:{lengths:1 lengths:1 values:"66"}} keyspace:"ks" shard:"80-"} keyspace:"ks" shard:"80-"
Event 10; type:ROW row_event:{table_name:"ks.t1" row_changes:{after:{lengths:1 lengths:1 values:"77"}} keyspace:"ks" shard:"80-"} keyspace:"ks" shard:"80-"
Event 11; type:ROW row_event:{table_name:"ks.t1" row_changes:{after:{lengths:1 lengths:1 values:"88"}} keyspace:"ks" shard:"80-"} keyspace:"ks" shard:"80-"
Event 12; type:VGTID vgtid:{shard_gtids:{keyspace:"ks" shard:"-80" gtid:"MySQL56/82178ac0-6666-11ed-b225-e84ab9c3a7f8:1-75" table_p_ks:{table_name:"t1" lastpk:{rows:{lengths:1 values:"5"}}}} shard_gtids:{keyspace:"ks" shard:"80-" gtid:"MySQL56/82178ac0-6666-11ed-b225-e84ab9c3a7f8:1-75" table_p_ks:{table_name:"t1" lastpk:{rows:{lengths:1 values:"8"}}}}} keyspace:"ks" shard:"80-"
Event 13; type:COMMIT keyspace:"ks" shard:"80-"
Event 14; type:BEGIN keyspace:"ks" shard:"80-"
Event 15; type:VGTID vgtid:{shard_gtids:{keyspace:"ks" shard:"-80" gtid:"MySQL56/82178ac0-6666-11ed-b225-e84ab9c3a7f8:1-75" table_p_ks:{table_name:"t1" lastpk:{rows:{lengths:1 values:"5"}}}} shard_gtids:{keyspace:"ks" shard:"80-" gtid:"MySQL56/82178ac0-6666-11ed-b225-e84ab9c3a7f8:1-75"}} keyspace:"ks" shard:"80-"
Event 16; type:COMMIT keyspace:"ks" shard:"80-"
===END===
    vstream_test.go:222: TestVStreamCopyBasic was successful
--- PASS: TestVStreamCopyBasic (0.01s)
PASS

# Apparently, Event 6, Event 13, and Event 14 are not expected.
## I guess that these rows are inserted at https://github.com/vitessio/vitess/blob/4c33b2b5a90dc200e6cc649681cbace2520725bd/go/vt/vtgate/endtoend/lookup_test.go#L292.
$ go test -v -count=1 vitess.io/vitess/go/vt/vtgate/endtoend --alsologtostderr
...
===START===
Event 0; type:BEGIN keyspace:"ks" shard:"80-"
Event 1; type:FIELD field_event:{table_name:"ks.t1" fields:{name:"id1" type:INT64 table:"t1" org_table:"t1" database:"vt_ks_80-" org_name:"id1" column_length:20 charset:63 flags:53251} fields:{name:"id2" type:INT64 table:"t1" org_table:"t1" database:"vt_ks_80-" org_name:"id2" column_length:20 charset:63 flags:32768} keyspace:"ks" shard:"80-"} keyspace:"ks" shard:"80-"
Event 2; type:VGTID vgtid:{shard_gtids:{keyspace:"ks" shard:"-80" table_p_ks:{table_name:"t1" lastpk:{fields:{name:"id1" type:INT32} rows:{lengths:1 values:"4"}}}} shard_gtids:{keyspace:"ks" shard:"80-" gtid:"MySQL56/2ac38034-6667-11ed-a179-dd231a1931a8:1-148" table_p_ks:{table_name:"t1" lastpk:{fields:{name:"id1" type:INT32} rows:{lengths:1 values:"4"}}}}} keyspace:"ks" shard:"80-"
Event 3; type:ROW row_event:{table_name:"ks.t1" row_changes:{after:{lengths:1 lengths:1 values:"66"}} keyspace:"ks" shard:"80-"} keyspace:"ks" shard:"80-"
Event 4; type:ROW row_event:{table_name:"ks.t1" row_changes:{after:{lengths:1 lengths:1 values:"77"}} keyspace:"ks" shard:"80-"} keyspace:"ks" shard:"80-"
Event 5; type:ROW row_event:{table_name:"ks.t1" row_changes:{after:{lengths:1 lengths:1 values:"88"}} keyspace:"ks" shard:"80-"} keyspace:"ks" shard:"80-"
Event 6; type:ROW row_event:{table_name:"ks.t1" row_changes:{after:{lengths:2 lengths:2 values:"5060"}} keyspace:"ks" shard:"80-"} keyspace:"ks" shard:"80-"
Event 7; type:VGTID vgtid:{shard_gtids:{keyspace:"ks" shard:"-80" table_p_ks:{table_name:"t1" lastpk:{fields:{name:"id1" type:INT32} rows:{lengths:1 values:"4"}}}} shard_gtids:{keyspace:"ks" shard:"80-" gtid:"MySQL56/2ac38034-6667-11ed-a179-dd231a1931a8:1-148" table_p_ks:{table_name:"t1" lastpk:{rows:{lengths:2 values:"50"}}}}} keyspace:"ks" shard:"80-"
Event 8; type:COMMIT keyspace:"ks" shard:"80-"
Event 9; type:BEGIN keyspace:"ks" shard:"-80"
Event 10; type:FIELD field_event:{table_name:"ks.t1" fields:{name:"id1" type:INT64 table:"t1" org_table:"t1" database:"vt_ks_-80" org_name:"id1" column_length:20 charset:63 flags:53251} fields:{name:"id2" type:INT64 table:"t1" org_table:"t1" database:"vt_ks_-80" org_name:"id2" column_length:20 charset:63 flags:32768} keyspace:"ks" shard:"-80"} keyspace:"ks" shard:"-80"
Event 11; type:VGTID vgtid:{shard_gtids:{keyspace:"ks" shard:"-80" gtid:"MySQL56/2ac38034-6667-11ed-a179-dd231a1931a8:1-148" table_p_ks:{table_name:"t1" lastpk:{fields:{name:"id1" type:INT32} rows:{lengths:1 values:"4"}}}} shard_gtids:{keyspace:"ks" shard:"80-" gtid:"MySQL56/2ac38034-6667-11ed-a179-dd231a1931a8:1-148" table_p_ks:{table_name:"t1" lastpk:{rows:{lengths:2 values:"50"}}}}} keyspace:"ks" shard:"-80"
Event 12; type:ROW row_event:{table_name:"ks.t1" row_changes:{after:{lengths:1 lengths:1 values:"55"}} keyspace:"ks" shard:"-80"} keyspace:"ks" shard:"-80"
Event 13; type:ROW row_event:{table_name:"ks.t1" row_changes:{after:{lengths:2 lengths:2 values:"1020"}} keyspace:"ks" shard:"-80"} keyspace:"ks" shard:"-80"
Event 14; type:ROW row_event:{table_name:"ks.t1" row_changes:{after:{lengths:2 lengths:2 values:"3040"}} keyspace:"ks" shard:"-80"} keyspace:"ks" shard:"-80"
Event 15; type:VGTID vgtid:{shard_gtids:{keyspace:"ks" shard:"-80" gtid:"MySQL56/2ac38034-6667-11ed-a179-dd231a1931a8:1-148" table_p_ks:{table_name:"t1" lastpk:{rows:{lengths:2 values:"30"}}}} shard_gtids:{keyspace:"ks" shard:"80-" gtid:"MySQL56/2ac38034-6667-11ed-a179-dd231a1931a8:1-148" table_p_ks:{table_name:"t1" lastpk:{rows:{lengths:2 values:"50"}}}}} keyspace:"ks" shard:"-80"
Event 16; type:COMMIT keyspace:"ks" shard:"-80"
===END===
    vstream_test.go:222: TestVStreamCopyBasic was successful
--- PASS: TestVStreamCopyBasic (0.01s)

Since this discrepancy caused the test failure in my PR, I fixed it.

@yoheimuta
Copy link
Contributor Author

yoheimuta commented Nov 18, 2022

All checks have passed. Thank you!

rohit-nayak-ps
rohit-nayak-ps approved these changes Nov 29, 2022
View reviewed changes
Copy link
Member

@rohit-nayak-ps rohit-nayak-ps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Thanks for fixing the tests as well.

mattlord
mattlord reviewed Nov 29, 2022
View reviewed changes
Copy link
Member

@mattlord mattlord left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on this! I had some minor questions/comments/suggestions. I'll let you make the final call on the subjective preferences noted. We can quickly address these and then get this merged! Feel free to respond here or reach out on slack, whichever you prefer. ❤️

@mattlord
Copy link
Member

mattlord commented Nov 29, 2022

A note about docs... I'm on the fence regarding whether or not this should be in the release notes. @rohit-nayak-ps, what do you think?

I also think that we should probably add info here about these new events and what they signify: vitessio/website#1216 I'll link the two PRs.

@mattlord mattlord mentioned this pull request Nov 29, 2022
Merged
@yoheimuta
VSCopy: refactor the function signature to be clearer
f4ac20a 
Signed-off-by: yoheimuta <yoheimuta@gmail.com>
@yoheimuta
VSCopy: refactor the VEvents sorter to be simpler
149a150 
Signed-off-by: yoheimuta <yoheimuta@gmail.com>
@yoheimuta
VSCopy: refactor to stop the sorter from including a fully copied event
076327d 
Signed-off-by: yoheimuta <yoheimuta@gmail.com>
@yoheimuta
Copy link
Contributor Author

yoheimuta commented Nov 30, 2022

@mattlord I applied all your suggestions and answered your question.
Can you check it out?

I think it looks possible to pass the failed test by rerunning https://github.com/vitessio/vitess/actions/runs/3580407547/jobs/6022506484.

@rohit-nayak-ps
Copy link
Member

rohit-nayak-ps commented Nov 30, 2022

I think it looks possible to pass the failed test by rerunning https://github.com/vitessio/vitess/actions/runs/3580407547/jobs/6022506484.

Yes, it succeeded on a retry!

@mattlord mattlord self-requested a review November 30, 2022 17:56
mattlord
mattlord approved these changes Nov 30, 2022
View reviewed changes
Copy link
Member

@mattlord mattlord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really good. Thanks again for fixing that long flaky test too! 🙂

Regarding the docs, I think we're good as:

  1. We have the existing release notes section which covers the new feature in general and links to the in progress new docs page: https://github.com/vitessio/vitess/blob/main/doc/releasenotes/16_0_0_summary.md#vstream-copy-resume
  2. We have that same open docs PR that we can add to for these new events: vitessio/website#1216

@mattlord mattlord merged commit 4ff02c0 into vitessio:main Nov 30, 2022
@yoheimuta yoheimuta deleted the send-copy-done-event branch December 1, 2022 10:58
@pbibra pbibra mentioned this pull request Sep 13, 2023
Merged
timvaillancourt pushed a commit to slackhq/vitess that referenced this pull request May 27, 2024
@yoheimuta @rohit-nayak-ps @timvaillancourt
VSCopy: Send COPY_COMPLETED events when the copy operation is done (v…
0e39ddd 
…itessio#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>
@timvaillancourt timvaillancourt mentioned this pull request May 27, 2024
Merged
4 tasks
timvaillancourt added a commit to slackhq/vitess that referenced this pull request May 28, 2024
@timvaillancourt @pbibra
@yoheimuta @mattlord @rohit-nayak-ps
slack-15.0: VStreams fix backports/patches, pt. 1 (#373)
8125303 
* 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>
@mattlord mattlord mentioned this pull request Sep 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattlord mattlord mattlord approved these changes

@rohit-nayak-ps rohit-nayak-ps rohit-nayak-ps approved these changes

@deepthi deepthi Awaiting requested review from deepthi

@harshit-gangal harshit-gangal Awaiting requested review from harshit-gangal harshit-gangal is a code owner

@systay systay Awaiting requested review from systay systay is a code owner

@shlomi-noach shlomi-noach Awaiting requested review from shlomi-noach shlomi-noach is a code owner

@frouioui frouioui Awaiting requested review from frouioui frouioui is a code owner

@GuptaManan100 GuptaManan100 Awaiting requested review from GuptaManan100

@ajm188 ajm188 Awaiting requested review from ajm188

@notfelineit notfelineit Awaiting requested review from notfelineit

Assignees

No one assigned

Labels

Component: VReplication Type: Enhancement Logical improvement (somewhere between a bug and feature)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yoheimuta @mattlord @rohit-nayak-ps