From d7bb90814892c35a2f4e764ea2c9242cd0e9a6bd Mon Sep 17 00:00:00 2001 From: jaime Date: Tue, 24 Jan 2023 10:01:46 +0800 Subject: [PATCH] Fix rename collection data race (#21827) Signed-off-by: jaime --- internal/proxy/impl.go | 2 +- internal/proxy/impl_test.go | 2 +- internal/rootcoord/meta_table.go | 15 ++++++++------- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/internal/proxy/impl.go b/internal/proxy/impl.go index b4498120f1..357d247bac 100644 --- a/internal/proxy/impl.go +++ b/internal/proxy/impl.go @@ -4428,7 +4428,7 @@ func (node *Proxy) RenameCollection(ctx context.Context, req *milvuspb.RenameCol return &commonpb.Status{ ErrorCode: commonpb.ErrorCode_IllegalCollectionName, Reason: err.Error(), - }, err + }, nil } req.Base = commonpbutil.NewMsgBase( diff --git a/internal/proxy/impl_test.go b/internal/proxy/impl_test.go index 096bba2105..bf71d270b1 100644 --- a/internal/proxy/impl_test.go +++ b/internal/proxy/impl_test.go @@ -160,7 +160,7 @@ func TestProxyRenameCollection(t *testing.T) { node.stateCode.Store(commonpb.StateCode_Healthy) ctx := context.Background() resp, err := node.RenameCollection(ctx, &milvuspb.RenameCollectionRequest{NewName: "$#^%#&#$*!)#@!"}) - assert.Error(t, err) + assert.NoError(t, err) assert.Equal(t, commonpb.ErrorCode_IllegalCollectionName, resp.GetErrorCode()) }) diff --git a/internal/rootcoord/meta_table.go b/internal/rootcoord/meta_table.go index 59b666d9ec..8acbebc030 100644 --- a/internal/rootcoord/meta_table.go +++ b/internal/rootcoord/meta_table.go @@ -330,9 +330,10 @@ func (mt *MetaTable) getCollectionByIDInternal(ctx context.Context, collectionID func (mt *MetaTable) GetCollectionByName(ctx context.Context, collectionName string, ts Timestamp) (*model.Collection, error) { mt.ddLock.RLock() defer mt.ddLock.RUnlock() + return mt.getCollectionByNameInternal(ctx, collectionName, ts) +} - var collectionID UniqueID - +func (mt *MetaTable) getCollectionByNameInternal(ctx context.Context, collectionName string, ts Timestamp) (*model.Collection, error) { collectionID, ok := mt.collAlias2ID[collectionName] if ok { return mt.getCollectionByIDInternal(ctx, collectionID, ts, false) @@ -447,10 +448,10 @@ func (mt *MetaTable) AlterCollection(ctx context.Context, oldColl *model.Collect } func (mt *MetaTable) RenameCollection(ctx context.Context, oldName string, newName string, ts Timestamp) error { - mt.ddLock.RLock() - defer mt.ddLock.RUnlock() - ctx = contextutil.WithTenantID(ctx, Params.CommonCfg.ClusterName.GetValue()) + mt.ddLock.Lock() + defer mt.ddLock.Unlock() + ctx = contextutil.WithTenantID(ctx, Params.CommonCfg.ClusterName.GetValue()) log := log.Ctx(ctx).With(zap.String("oldName", oldName), zap.String("newName", newName)) //old collection should not be an alias @@ -461,7 +462,7 @@ func (mt *MetaTable) RenameCollection(ctx context.Context, oldName string, newNa } // check new collection already exists - newColl, err := mt.GetCollectionByName(ctx, newName, ts) + newColl, err := mt.getCollectionByNameInternal(ctx, newName, ts) if newColl != nil { return fmt.Errorf("duplicated new collection name :%s with other collection name or alias", newName) } @@ -471,7 +472,7 @@ func (mt *MetaTable) RenameCollection(ctx context.Context, oldName string, newNa } // get old collection meta - oldColl, err := mt.GetCollectionByName(ctx, oldName, ts) + oldColl, err := mt.getCollectionByNameInternal(ctx, oldName, ts) if err != nil { return err }