From 281ff0461980ef0e7ba2bc5805d910b62d709730 Mon Sep 17 00:00:00 2001 From: Andrew Mason Date: Sun, 31 Jan 2021 10:24:57 -0500 Subject: [PATCH] Add protoutil package, refactor ISP to use it We're going to be dealing with a lot of duration pb types. Signed-off-by: Andrew Mason --- .github/CODEOWNERS | 1 + go/protoutil/doc.go | 25 ++++++++ go/protoutil/duration.go | 42 +++++++++++++ go/protoutil/duration_test.go | 83 ++++++++++++++++++++++++++ go/vt/vtctl/grpcvtctldserver/server.go | 15 ++--- 5 files changed, 156 insertions(+), 10 deletions(-) create mode 100644 go/protoutil/doc.go create mode 100644 go/protoutil/duration.go create mode 100644 go/protoutil/duration_test.go diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 88eb14077c6..a825a23c862 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -7,6 +7,7 @@ /go/cmd/vtadmin @ajm188 @doeg /go/cmd/vtctldclient @ajm188 @doeg /go/mysql @harshit-gangal @systay +/go/protoutil @ajm188 /go/test/endtoend/onlineddl @shlomi-noach /go/test/endtoend/orchestrator @deepthi @shlomi-noach /go/test/endtoend/vtgate @harshit-gangal @systay diff --git a/go/protoutil/doc.go b/go/protoutil/doc.go new file mode 100644 index 00000000000..15a0aad339e --- /dev/null +++ b/go/protoutil/doc.go @@ -0,0 +1,25 @@ +/* +Copyright 2021 The Vitess Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +/* +Package protoutil provides helper functions for working with well-known protobuf +types. + +It aims to serve a purpose similar to packages topoproto and mysqlctlproto, but +for general, common types, rather than types related to a particular Vitess RPC +service. +*/ +package protoutil diff --git a/go/protoutil/duration.go b/go/protoutil/duration.go new file mode 100644 index 00000000000..fd1aea7a5d6 --- /dev/null +++ b/go/protoutil/duration.go @@ -0,0 +1,42 @@ +/* +Copyright 2021 The Vitess Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package protoutil + +import ( + "time" + + "github.com/golang/protobuf/ptypes" + + durationpb "github.com/golang/protobuf/ptypes/duration" +) + +// DurationFromProto converts a durationpb type to a time.Duration. It returns a +// three-tuple of (dgo, ok, err) where dgo is the go time.Duration, ok indicates +// whether the proto value was set, and err is set on failure to convert the +// proto value. +func DurationFromProto(dpb *durationpb.Duration) (time.Duration, bool, error) { + if dpb == nil { + return 0, false, nil + } + + dgo, err := ptypes.Duration(dpb) + if err != nil { + return 0, true, err + } + + return dgo, true, nil +} diff --git a/go/protoutil/duration_test.go b/go/protoutil/duration_test.go new file mode 100644 index 00000000000..e39e9fd3a2b --- /dev/null +++ b/go/protoutil/duration_test.go @@ -0,0 +1,83 @@ +/* +Copyright 2021 The Vitess Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package protoutil + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + + durationpb "github.com/golang/protobuf/ptypes/duration" +) + +func TestDurationFromProto(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in *durationpb.Duration + expected time.Duration + isOk bool + shouldErr bool + }{ + { + name: "success", + in: &durationpb.Duration{Seconds: 1000}, + expected: time.Second * 1000, + isOk: true, + shouldErr: false, + }, + { + name: "nil value", + in: nil, + expected: 0, + isOk: false, + shouldErr: false, + }, + { + name: "error", + in: &durationpb.Duration{ + // This is the max allowed seconds for a durationpb, plus 1. + Seconds: int64(10000*365.25*24*60*60) + 1, + }, + expected: 0, + isOk: true, + shouldErr: true, + }, + } + + for _, tt := range tests { + tt := tt + + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + actual, ok, err := DurationFromProto(tt.in) + if tt.shouldErr { + assert.Error(t, err) + assert.Equal(t, tt.isOk, ok, "expected (_, ok, _) = DurationFromProto; to be ok = %v", tt.isOk) + + return + } + + assert.NoError(t, err) + assert.Equal(t, tt.expected, actual) + assert.Equal(t, tt.isOk, ok, "expected (_, ok, _) = DurationFromProto; to be ok = %v", tt.isOk) + }) + } +} diff --git a/go/vt/vtctl/grpcvtctldserver/server.go b/go/vt/vtctl/grpcvtctldserver/server.go index c05580f8dd5..3c83f9cfd66 100644 --- a/go/vt/vtctl/grpcvtctldserver/server.go +++ b/go/vt/vtctl/grpcvtctldserver/server.go @@ -23,10 +23,10 @@ import ( "sync" "time" - "github.com/golang/protobuf/ptypes" "google.golang.org/grpc" "vitess.io/vitess/go/event" + "vitess.io/vitess/go/protoutil" "vitess.io/vitess/go/sqlescape" "vitess.io/vitess/go/vt/concurrency" "vitess.io/vitess/go/vt/log" @@ -365,15 +365,10 @@ func (s *VtctldServer) InitShardPrimary(ctx context.Context, req *vtctldatapb.In return nil, vterrors.Errorf(vtrpc.Code_INVALID_ARGUMENT, "shard field is required") } - var waitReplicasTimeout time.Duration - if req.WaitReplicasTimeout != nil { - wrt, err := ptypes.Duration(req.WaitReplicasTimeout) - if err != nil { - return nil, vterrors.Errorf(vtrpc.Code_INVALID_ARGUMENT, "cannot parse WaitReplicasTimeout; err = %v", err) - } - - waitReplicasTimeout = wrt - } else { + waitReplicasTimeout, ok, err := protoutil.DurationFromProto(req.WaitReplicasTimeout) + if err != nil { + return nil, err + } else if !ok { waitReplicasTimeout = time.Second * 30 }