-
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
Split clone using consistent snapshot #4541
Conversation
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
4564691
to
1d8fb20
Compare
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.
A couple of nits. It looks like you've cleaned up some of the code. I'll trust the tests for the correctness. It will be good if @tirsen eyeballed them also.
@@ -55,7 +55,7 @@ func main() { | |||
logutil.LogEvent(logger, e) | |||
}) | |||
if err != nil { | |||
log.Error(err) | |||
log.Errorf("%+v", err) |
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.
Is this intentional? Or did you change this for troubleshooting?
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.
I did change it for trouble shooting, but that does not mean that it needs to go back. %+v
includes the stacktrace, and I think that always makes sense to log for errors. I'll remove it from here and issue a separate PR with more of these changes.
@@ -23,6 +23,8 @@ import ( | |||
"strings" | |||
"time" | |||
|
|||
"github.com/golang/glog" |
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 should instead be vitess.io/vitess/go/vt/log
. It allows us to remap the logging functions as needed.
go/vt/worker/split_clone.go
Outdated
@@ -424,6 +435,29 @@ func (scw *SplitCloneWorker) Run(ctx context.Context) error { | |||
return nil | |||
} | |||
|
|||
func (scw *SplitCloneWorker) disableUniquenessCheckOnDestinationTablets(ctx context.Context) error { |
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.
I believe this is still questionable. I don't see it used in this PR. Should we remove it until it becomes official?
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.
Yes please remove it. I think it would be good but it's currently hard to make work.
Signed-off-by: Andres Taylor <[email protected]>
…clone * upstream/master: docker: don't chown nonexistent /vt dir mysqlctl: add backup test for rockdb and sdi files Adding a reset of the slave on blank restore Add a special plan type for impossible queries mysqlctl: add MyRocks dir/files to backups Use recommended version combination Remove trailing whitespace
go/vt/worker/split_clone_cmd.go
Outdated
<LABEL for="maxTPS">Maximum Write Transactions/second (If non-zero, writes on the destination will be throttled. Unlimited by default.): </LABEL> | ||
<INPUT type="text" id="maxTPS" name="maxTPS" value="{{.DefaultMaxTPS}}"></BR> | ||
<LABEL for="maxReplicationLag">Maximum Replication Lag Seconds (enables the adapative throttler. Disabled by default.): </LABEL> | ||
<INPUT type="text" id="maxReplicationLag" name="maxReplicationLag" value="{{.DefaultMaxReplicationLag}}"></BR> | ||
<INPUT type="hidden" name="keyspace" value="{{.Keyspace}}"/> | ||
<INPUT type="hidden" name="shard" value="{{.Shard}}"/> | ||
<LABEL for="useConsistentSnapshot">Use consistent snapshot during the offline cloning:</LABEL> | ||
<INPUT type="checkbox" id="useConsistentSnapshot" name="useConsistentSnapshot" value="false"><a href="https://dev.mysql.com/doc/refman/5.7/en/glossary.html#glos_consistent_read" target="_blank">?</a></BR> |
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.
that value needs to be value="true"
Signed-off-by: Andres Taylor <[email protected]>
No description provided.