From 81ef344f42339e4668015e50023037c27c207f5e Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Sat, 18 May 2024 01:36:30 -0400 Subject: [PATCH] Fix error handling and use int64 Signed-off-by: Matt Lord --- go/vt/topo/conn.go | 2 +- go/vt/topo/consultopo/file.go | 2 +- go/vt/topo/etcd2topo/file.go | 4 +-- go/vt/topo/faketopo/faketopo.go | 2 +- go/vt/topo/memorytopo/file.go | 2 +- go/vt/topo/stats_conn.go | 2 +- go/vt/topo/stats_conn_test.go | 2 +- go/vt/topo/zk2topo/file.go | 2 +- go/vt/vtctl/grpcvtctldserver/server.go | 42 ++++++++++++-------------- 9 files changed, 28 insertions(+), 32 deletions(-) diff --git a/go/vt/topo/conn.go b/go/vt/topo/conn.go index bd39e518577..fe8d245f65e 100644 --- a/go/vt/topo/conn.go +++ b/go/vt/topo/conn.go @@ -76,7 +76,7 @@ type Conn interface { // filePath is a path relative to the root directory of the cell. // Can return ErrNoNode if the file doesn't exist and ErrNoVersion // if the version doesn't exist. - GetVersion(ctx context.Context, filePath string, version Version) ([]byte, error) + GetVersion(ctx context.Context, filePath string, version int64) ([]byte, error) // List returns KV pairs, along with metadata like the version, for // entries where the key contains the specified prefix. diff --git a/go/vt/topo/consultopo/file.go b/go/vt/topo/consultopo/file.go index eee002cb5fb..b38b48863b6 100644 --- a/go/vt/topo/consultopo/file.go +++ b/go/vt/topo/consultopo/file.go @@ -99,7 +99,7 @@ func (s *Server) Get(ctx context.Context, filePath string) ([]byte, topo.Version } // GetVersion is part of topo.Conn interface. -func (s *Server) GetVersion(ctx context.Context, filePath string, version topo.Version) ([]byte, error) { +func (s *Server) GetVersion(ctx context.Context, filePath string, version int64) ([]byte, error) { return nil, topo.NewError(topo.NoImplementation, "GetVersion not supported in consul topo") } diff --git a/go/vt/topo/etcd2topo/file.go b/go/vt/topo/etcd2topo/file.go index 071c9ac3e8e..63aa7de5fcb 100644 --- a/go/vt/topo/etcd2topo/file.go +++ b/go/vt/topo/etcd2topo/file.go @@ -89,10 +89,10 @@ func (s *Server) Get(ctx context.Context, filePath string) ([]byte, topo.Version } // GetVersion is part of the topo.Conn interface. -func (s *Server) GetVersion(ctx context.Context, filePath string, version topo.Version) ([]byte, error) { +func (s *Server) GetVersion(ctx context.Context, filePath string, version int64) ([]byte, error) { nodePath := path.Join(s.root, filePath) - resp, err := s.cli.Get(ctx, nodePath, clientv3.WithRev(int64(version.(EtcdVersion)))) + resp, err := s.cli.Get(ctx, nodePath, clientv3.WithRev(version)) if err != nil { return nil, convertError(err, nodePath) } diff --git a/go/vt/topo/faketopo/faketopo.go b/go/vt/topo/faketopo/faketopo.go index 111e378f495..52a2a41c5df 100644 --- a/go/vt/topo/faketopo/faketopo.go +++ b/go/vt/topo/faketopo/faketopo.go @@ -254,7 +254,7 @@ func (f *FakeConn) Get(ctx context.Context, filePath string) ([]byte, topo.Versi } // GetVersion is part of topo.Conn interface. -func (f *FakeConn) GetVersion(ctx context.Context, filePath string, version topo.Version) ([]byte, error) { +func (f *FakeConn) GetVersion(ctx context.Context, filePath string, version int64) ([]byte, error) { return nil, topo.NewError(topo.NoImplementation, "GetVersion not supported in fake topo") } diff --git a/go/vt/topo/memorytopo/file.go b/go/vt/topo/memorytopo/file.go index 50473de7c20..5bf4e591de7 100644 --- a/go/vt/topo/memorytopo/file.go +++ b/go/vt/topo/memorytopo/file.go @@ -191,7 +191,7 @@ func (c *Conn) Get(ctx context.Context, filePath string) ([]byte, topo.Version, } // GetVersion is part of topo.Conn interface. -func (c *Conn) GetVersion(ctx context.Context, filePath string, version topo.Version) ([]byte, error) { +func (c *Conn) GetVersion(ctx context.Context, filePath string, version int64) ([]byte, error) { return nil, topo.NewError(topo.NoImplementation, "GetVersion not supported in memory topo") } diff --git a/go/vt/topo/stats_conn.go b/go/vt/topo/stats_conn.go index 10c543d46d6..34c45d793ac 100644 --- a/go/vt/topo/stats_conn.go +++ b/go/vt/topo/stats_conn.go @@ -116,7 +116,7 @@ func (st *StatsConn) Get(ctx context.Context, filePath string) ([]byte, Version, } // GetVersion is part of the Conn interface. -func (st *StatsConn) GetVersion(ctx context.Context, filePath string, version Version) ([]byte, error) { +func (st *StatsConn) GetVersion(ctx context.Context, filePath string, version int64) ([]byte, error) { startTime := time.Now() statsKey := []string{"GetVersion", st.cell} defer topoStatsConnTimings.Record(statsKey, startTime) diff --git a/go/vt/topo/stats_conn_test.go b/go/vt/topo/stats_conn_test.go index 173679cee3e..78f9cc6eb72 100644 --- a/go/vt/topo/stats_conn_test.go +++ b/go/vt/topo/stats_conn_test.go @@ -74,7 +74,7 @@ func (st *fakeConn) Get(ctx context.Context, filePath string) (bytes []byte, ver } // GetVersion is part of the Conn interface -func (st *fakeConn) GetVersion(ctx context.Context, filePath string, version Version) (bytes []byte, err error) { +func (st *fakeConn) GetVersion(ctx context.Context, filePath string, version int64) (bytes []byte, err error) { if filePath == "error" { return bytes, fmt.Errorf("Dummy error") diff --git a/go/vt/topo/zk2topo/file.go b/go/vt/topo/zk2topo/file.go index 5f8f48b7eda..b2152964dea 100644 --- a/go/vt/topo/zk2topo/file.go +++ b/go/vt/topo/zk2topo/file.go @@ -89,7 +89,7 @@ func (zs *Server) Get(ctx context.Context, filePath string) ([]byte, topo.Versio } // GetVersion is part of topo.Conn interface. -func (zs *Server) GetVersion(ctx context.Context, filePath string, version topo.Version) ([]byte, error) { +func (zs *Server) GetVersion(ctx context.Context, filePath string, version int64) ([]byte, error) { return nil, topo.NewError(topo.NoImplementation, "GetVersion not supported in ZK2 topo") } diff --git a/go/vt/vtctl/grpcvtctldserver/server.go b/go/vt/vtctl/grpcvtctldserver/server.go index 25d4b555aa7..14d9ce576cb 100644 --- a/go/vt/vtctl/grpcvtctldserver/server.go +++ b/go/vt/vtctl/grpcvtctldserver/server.go @@ -55,7 +55,6 @@ import ( "vitess.io/vitess/go/vt/schemamanager" "vitess.io/vitess/go/vt/sqlparser" "vitess.io/vitess/go/vt/topo" - "vitess.io/vitess/go/vt/topo/etcd2topo" "vitess.io/vitess/go/vt/topo/topoproto" "vitess.io/vitess/go/vt/topotools" "vitess.io/vitess/go/vt/topotools/events" @@ -2280,12 +2279,7 @@ func (s *VtctldServer) GetTopologyPath(ctx context.Context, req *vtctldatapb.Get } // Otherwise, delegate to getTopologyCell to parse the path and return the cell there. - var version topo.Version - if req.GetVersion() != 0 { - // Getting specific versions is only supported with the etcd2topo today. - version = etcd2topo.EtcdVersion(req.GetVersion()) - } - cell, err := s.getTopologyCell(ctx, req.GetPath(), version, req.GetAsJson()) + cell, err := s.getTopologyCell(ctx, req.GetPath(), req.GetVersion(), req.GetAsJson()) if err != nil { return nil, err } @@ -5072,7 +5066,7 @@ func StartServer(s *grpc.Server, env *vtenv.Environment, ts *topo.Server) { } // getTopologyKey is a helper method that returns a topology key given its path. -func (s *VtctldServer) getTopologyCell(ctx context.Context, cellPath string, version topo.Version, asJSON bool) (*vtctldatapb.TopologyCell, error) { +func (s *VtctldServer) getTopologyCell(ctx context.Context, cellPath string, version int64, asJSON bool) (*vtctldatapb.TopologyCell, error) { // extract cell and relative path parts := strings.Split(cellPath, "/") if parts[0] != "" || len(parts) < 2 { @@ -5089,37 +5083,39 @@ func (s *VtctldServer) getTopologyCell(ctx context.Context, cellPath string, ver return nil, err } - if version != nil { - if data, err := conn.GetVersion(ctx, relativePath, version); err == nil { + if version != 0 { + data, err := conn.GetVersion(ctx, relativePath, version) + if err == nil { result, err := topo.DecodeContent(relativePath, data, asJSON) if err != nil { - err := vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "error decoding file content for cell %s (version: %s): %v", cellPath, version, err) - return nil, err + return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "error decoding file content for cell %s (version: %d): %v", cellPath, version, err) } topoCell.Data = result - topoCell.Version, err = strconv.ParseInt(version.String(), 10, 64) - if err != nil { - return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "error decoding file version for cell %s (version: %s): %v", cellPath, version, err) - } - // since there is data at this cell, it cannot be a directory cell - // so we can early return the topocell + topoCell.Version = version + // Since there is data at this cell, it cannot be a directory cell + // so we can early return the topocell. return &topoCell, nil } + if topo.IsErrType(err, topo.NoImplementation) { + // The topo server does not support GetVersion. + return nil, err + } + // Otherwise this may be a "directory" with children at this path so + // we carry on. } else { if data, curVersion, err := conn.Get(ctx, relativePath); err == nil { result, err := topo.DecodeContent(relativePath, data, asJSON) if err != nil { - err := vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "error decoding file content for cell %s: %v", cellPath, err) - return nil, err + return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "error decoding file content for cell %s: %v", cellPath, err) } topoCell.Data = result topoCell.Version, err = strconv.ParseInt(curVersion.String(), 10, 64) if err != nil { - return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "error decoding file version for cell %s (version: %s): %v", cellPath, version, err) + return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "error decoding file version for cell %s (version: %d): %v", cellPath, version, err) } - // since there is data at this cell, it cannot be a directory cell - // so we can early return the topocell + // Since there is data at this cell, it cannot be a directory cell + // so we can early return the topocell. return &topoCell, nil } }