Fix error handling and use int64

Signed-off-by: Matt Lord <[email protected]>
vitessio · May 18, 2024 · 81ef344 · 81ef344
1 parent e5bef58
commit 81ef344
Show file tree

Hide file tree

Showing 9 changed files with 28 additions and 32 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
 		}
 	}