feature: support truncate collection #32994

PowderLi · 2024-05-11T16:52:44Z

sre-ci-robot · 2024-05-11T16:53:07Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: PowderLi
To complete the pull request process, please assign yanliang567 after the PR has been reviewed.
You can assign the PR to them by writing /assign @yanliang567 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mergify · 2024-05-11T16:53:23Z

@PowderLi

Invalid PR Title Format Detected

Your PR submission does not adhere to our required standards. To ensure clarity and consistency, please meet the following criteria:

Title Format: The PR title must begin with one of these prefixes:

feat: for introducing a new feature.
fix: for bug fixes.
enhance: for improvements to existing functionality.
test: for add tests to existing functionality.
doc: for modifying documentation.
auto: for the pull request from bot.

Description Requirement: The PR must include a non-empty description, detailing the changes and their impact.

Required Title Structure:

[Type]: [Description of the PR]

Where Type is one of feat, fix, enhance, test or doc.

Example:

enhance: improve search performance significantly

Please review and update your PR to comply with these guidelines.

mergify · 2024-05-11T17:02:32Z

@PowderLi E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

mergify · 2024-05-11T17:44:31Z

@PowderLi ut workflow job failed, comment rerun ut can trigger the job again.

mergify · 2024-05-13T10:10:17Z

@PowderLi ut workflow job failed, comment rerun ut can trigger the job again.

mergify · 2024-05-13T10:34:01Z

@PowderLi E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

codecov · 2024-05-13T12:28:01Z

Codecov Report

Attention: Patch coverage is 95.78686% with 34 lines in your changes are missing coverage. Please review.

Project coverage is 82.31%. Comparing base (12e8c6c) to head (71676fe).
Report is 52 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #32994      +/-   ##
==========================================
+ Coverage   82.20%   82.31%   +0.10%     
==========================================
  Files        1009     1012       +3     
  Lines      128884   129688     +804     
==========================================
+ Hits       105945   106747     +802     
+ Misses      18952    18930      -22     
- Partials     3987     4011      +24

Files	Coverage Δ
internal/datacoord/index_meta.go	`98.32% <100.00%> (+0.50%)`	⬆️
internal/datacoord/index_service.go	`95.83% <100.00%> (+1.11%)`	⬆️
internal/distributed/datacoord/client/client.go	`98.00% <100.00%> (+0.04%)`	⬆️
internal/distributed/datacoord/service.go	`89.03% <100.00%> (+0.19%)`	⬆️
internal/distributed/rootcoord/client/client.go	`93.81% <100.00%> (+0.12%)`	⬆️
internal/distributed/rootcoord/service.go	`81.88% <100.00%> (+0.14%)`	⬆️
internal/metastore/catalog.go	`100.00% <ø> (ø)`
internal/proxy/impl.go	`86.58% <100.00%> (+0.15%)`	⬆️
internal/proxy/task.go	`88.05% <100.00%> (+0.29%)`	⬆️
internal/rootcoord/alter_alias_task.go	`100.00% <100.00%> (ø)`
... and 15 more

... and 35 files with indirect coverage changes

mergify · 2024-05-13T17:35:03Z

@PowderLi E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

mergify · 2024-05-20T15:15:13Z

@PowderLi E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

Signed-off-by: PowderLi <min.li@zilliz.com>

longjiquan · 2024-05-22T06:42:02Z

internal/datacoord/index_service.go

+// CreateIndexesForTemp returns all indexes created on provided collection.
+func (s *Server) CreateIndexesForTemp(ctx context.Context, req *indexpb.CollectionWithTempRequest) (*commonpb.Status, error) {
+	log := log.Ctx(ctx).With(zap.Int64("collection", req.CollectionID), zap.Int64("tempCollection", req.TempCollectionID))
+	log.Info("receive CopyIndexes to temp collection request")


We should also use info level in the following log.

longjiquan · 2024-05-22T06:47:00Z

internal/datacoord/index_service.go

+	err := s.meta.indexMeta.CreateIndex(&model.Index{
+		CollectionID: req.CollectionID,
+		IndexID:      int64(0),
+		IsDeleted:    true,


This will lead the indexes of the collection removed eventually. I think we can't do this since truncating a collection is not promised to be successful.

guard index
forbid new index operations happened in the same collection
if truncate fail, the guard index will be remove while we drop the temp collection

Make the special IndexID 0 as a constant for better readability.

longjiquan · 2024-05-22T06:47:38Z

internal/datacoord/index_service.go

+		return merr.Status(err), nil
+	}
+	log.Debug("created a guard index(id=0) of target collection")
+	indexes := s.meta.indexMeta.GetIndexesForCollection(req.GetCollectionID(), "")


Only copying all these indexes is enough?

what index missed you think?

Sry for my expressions. I mean I think copying all these indexes is enough, no need to create a guard index if we don't limit AlterIndex/CreateIndex in datacoord.

longjiquan · 2024-05-22T06:51:01Z

internal/datacoord/index_service.go

+}
+
+// DropIndexesForTemp returns all indexes created on provided collection.
+func (s *Server) DropIndexesForTemp(ctx context.Context, req *indexpb.CollectionWithTempRequest) (*commonpb.Status, error) {


We can reuse the datacoord.DropIndex directly.

we need to clear the guard index in the target collection whatever truncate success or fail

longjiquan · 2024-05-22T06:55:53Z

internal/metastore/kv/datacoord/kv_catalog.go

+			return err
+		}
+		kvMap[key] = string(value)
+		log.Ctx(ctx).Debug("add new indexes",


Use info level.

longjiquan · 2024-05-22T07:12:36Z

internal/rootcoord/broker.go

@@ -58,6 +58,7 @@ type Broker interface {
 	GcConfirm(ctx context.Context, collectionID, partitionID UniqueID) bool

 	DropCollectionIndex(ctx context.Context, collID UniqueID, partIDs []UniqueID) error
+	DropTempCollectionIndexes(ctx context.Context, collID UniqueID, tempCollID UniqueID) error


DropCollectionIndex is enough.

longjiquan · 2024-05-22T07:19:26Z

internal/rootcoord/copy_collection_task.go

+	return t.assignChannels()
+}
+
+func (t *copyCollectionTask) genCreateCollectionMsg(ctx context.Context, ts uint64) *ms.MsgPack {


It will be wonderful if we can make this method a util function, since the createCollectionTask can reuse this.

longjiquan · 2024-05-22T07:22:22Z

internal/rootcoord/copy_collection_task.go

+	"github.com/milvus-io/milvus/pkg/util/typeutil"
+)
+
+type copyCollectionTask struct {


From my perspective, copyCollectionTask did almost the same things with createCollectionTask except the collection name, can we combine these two tasks into one?

longjiquan · 2024-05-22T08:34:31Z

internal/rootcoord/root_coord.go

+		log.Info("failed to release temp collection", zap.Error(err))
+		return merr.Status(err), nil
+	}
+	_, err = c.garbageCollector.GcCollectionData(ctx, t.collInfo)


Wati for this ts to be synced.

In fact, we can make this a redo task, which is very useful to recycle the resources when any error is encounted, especially for releasing collection.

Just similar to dropping a collection.

longjiquan · 2024-05-22T08:39:09Z

internal/rootcoord/root_coord.go

+	}
+	defer func() {
+		log.Debug("drop the temp collection async")
+		c.dropCollectionForTemp(ctx, in, t.collectionID, t.tempCollectionID)


Why we should do this even if the truncating operation is done?

I think we can provide a abstraction which combine the undoTask and redoTask for the truncating workflow, like below:

type generalTask struct{ undoTask redoTask } func (t *generalTask) Execute() error { ReturnErrIf(t.undoTask.Execute()) go t.redoTask.Execute() return nil }

And truncating a collection in fact can be simplified to:

undo.AddStep(CreateTempCollection, RemoveTempCollection) // 1 undo.AddStep(CreateIndexesForCollection, nullstep) // 2 undo.AddStep(LoadTempCollection, nullstep) // 3 undo.AddStep(ExchangeCollections, nullstep) // 4 redo.AddStep(RemoveOriginalCollection) // 5

Consider the recovery and atomicity:

Fail at step 1, no temp collection was created;

Fail at step 2, the step executor will execute the undo step of 1, RemoveTempCollection, which will drop the indexes and releasing the temp collection, rootcoord will also do this when it starts;

Fail at 3 or 4, same with 2;

We can return when 4 is succeeded, because 4 is atomic so no any side effect will be brought;

After 4 is done, the gc of original collection is asynchronous;

sre-ci-robot added area/internal-api size/XXL Denotes a PR that changes 1000+ lines. labels May 11, 2024

sre-ci-robot requested review from bigsheeper and cydrain May 11, 2024 16:53

mergify bot added the dco-passed DCO check passed. label May 11, 2024

mergify bot added the do-not-merge/invalid-pr-format label May 11, 2024

PowderLi force-pushed the master-truncate-collection branch from 32201a0 to 1a50b05 Compare May 13, 2024 09:52

PowderLi force-pushed the master-truncate-collection branch from 1a50b05 to 3bc5311 Compare May 13, 2024 16:18

PowderLi force-pushed the master-truncate-collection branch from 3bc5311 to c4adc9c Compare May 14, 2024 09:53

sre-ci-robot added area/test sig/testing labels May 14, 2024

mergify bot added the ci-passed label May 14, 2024

PowderLi force-pushed the master-truncate-collection branch from c4adc9c to db28c03 Compare May 15, 2024 13:41

mergify bot added ci-passed and removed ci-passed labels May 15, 2024

PowderLi force-pushed the master-truncate-collection branch from db28c03 to ecfd633 Compare May 20, 2024 13:58

mergify bot removed the ci-passed label May 20, 2024

PowderLi force-pushed the master-truncate-collection branch from ecfd633 to 24e277b Compare May 21, 2024 04:09

mergify bot added the ci-passed label May 21, 2024

feature: support truncate collection

71676fe

Signed-off-by: PowderLi <min.li@zilliz.com>

PowderLi force-pushed the master-truncate-collection branch from 24e277b to 71676fe Compare May 21, 2024 13:13

mergify bot added ci-passed and removed ci-passed labels May 21, 2024

longjiquan suggested changes May 22, 2024

View reviewed changes

PowderLi requested a review from longjiquan May 27, 2024 05:05

PowderLi marked this pull request as draft May 27, 2024 05:05

sre-ci-robot added the do-not-merge/work-in-progress Don't merge even CI passed. label May 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: support truncate collection #32994

feature: support truncate collection #32994

PowderLi commented May 11, 2024

sre-ci-robot commented May 11, 2024

mergify bot commented May 11, 2024

mergify bot commented May 11, 2024

mergify bot commented May 11, 2024

mergify bot commented May 13, 2024

mergify bot commented May 13, 2024

codecov bot commented May 13, 2024 •

edited

mergify bot commented May 13, 2024

mergify bot commented May 20, 2024

longjiquan May 22, 2024

longjiquan May 22, 2024

PowderLi May 22, 2024

longjiquan May 22, 2024

longjiquan May 22, 2024

PowderLi May 22, 2024

longjiquan May 22, 2024

longjiquan May 22, 2024

PowderLi May 22, 2024

longjiquan May 22, 2024

longjiquan May 22, 2024

longjiquan May 22, 2024

longjiquan May 22, 2024

longjiquan May 22, 2024

longjiquan May 22, 2024

longjiquan May 22, 2024

longjiquan May 22, 2024

longjiquan May 22, 2024

feature: support truncate collection #32994

Are you sure you want to change the base?

feature: support truncate collection #32994

Conversation

PowderLi commented May 11, 2024

sre-ci-robot commented May 11, 2024

mergify bot commented May 11, 2024

mergify bot commented May 11, 2024

mergify bot commented May 11, 2024

mergify bot commented May 13, 2024

mergify bot commented May 13, 2024

codecov bot commented May 13, 2024 • edited

Codecov Report

mergify bot commented May 13, 2024

mergify bot commented May 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 13, 2024 •

edited