Clear space #3989

zhaohaifei · 2022-03-08T09:38:19Z

What type of PR is this?

bug
feature
enhancement

What problem(s) does this PR solve?

Issue(s) number:

fix #3679

Description:

How do you solve it?

Special notes for your reviewer, ex. impact of this fix, design document, etc:

Checklist:

Tests:

Unit test(positive and negative cases)
Function test
Performance test
N/A

Affects:

Documentation affected (Please add the label if documentation needs to be modified.)
Incompatibility (If it breaks the compatibility, please describe it and add the label.）
If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to be reflected in release notes and how to describe:

Added a clear space feature, which will clear space data and index data, but keep space schema and index schema.

Shylock-Hg · 2022-03-08T09:56:19Z

Any tests?

zhaohaifei · 2022-03-08T09:57:31Z

Any tests?

in the next pr. I'm worried that this PR is too big, no one wants to see it

src/interface/meta.thrift

critical27 · 2022-03-08T10:05:56Z

src/kvstore/NebulaStore.cpp

@@ -427,6 +427,24 @@ void NebulaStore::removeSpace(GraphSpaceID spaceId, bool isListener) {
  }
 }

+nebula::cpp2::ErrorCode NebulaStore::clearSpace(GraphSpaceID spaceId) {
+  folly::RWSpinLock::WriteHolder wh(&lock_);


The lock_ is not for DML, it is used for partition related modify, e.g. when add space or drop part.

Maybe you could find all kv_engine by NebulaStore::space, and get all parts by NebulaStore::allParts from kv_engine.

critical27 · 2022-03-08T10:06:01Z

src/kvstore/NebulaStore.cpp

+  auto spaceIt = this->spaces_.find(spaceId);
+  if (spaceIt != this->spaces_.end()) {
+    for (auto& part : spaceIt->second->parts_) {
+      auto ret = part.second->cleanup();


Perhaps you need grab the raft lock here. BTW, do we need to block write by meta?

src/meta/processors/admin/ClearSpaceProcessor.cpp

critical27 · 2022-03-08T10:09:04Z

src/meta/processors/admin/ClearSpaceProcessor.cpp

+  }
+
+  // 4. select the active hosts.
+  std::vector<HostAddr> selectedHosts;


If not all alive, just return error

critical27 · 2022-03-08T10:09:34Z

src/meta/processors/admin/ClearSpaceProcessor.cpp

+  }
+
+  // 3. Determine which hosts the space is distributed on.
+  std::vector<HostAddr> distributedOnHosts;


Maybe just use a unordered_set? Not so important

critical27 · 2022-03-08T10:10:40Z

src/meta/processors/admin/ClearSpaceProcessor.cpp

+
+void ClearSpaceProcessor::process(const cpp2::ClearSpaceReq& req) {
+  folly::SharedMutex::ReadHolder rHolder(LockUtils::snapshotLock());
+  folly::SharedMutex::WriteHolder holder(LockUtils::lock());


Why we need to use write lock here? BTW, do we really need to grab the snapshot lock here?

critical27

LGTM, good job

yixinglu · 2022-03-10T10:50:21Z

src/graph/executor/admin/SpaceExecutor.cpp

+      ->getMetaClient()
+      ->clearSpace(csNode->getSpaceName(), csNode->getIfExists())
+      .via(runner())
+      .thenValue([this, csNode, spaceIdRet, ftIndexes](StatusOr<bool> resp) {


better to move this ftIndexes into lambda function

yixinglu · 2022-03-10T11:11:21Z

src/meta/processors/admin/AdminClient.cpp

+        std::move(pro));
+  }
+
+  folly::collectAll(std::move(futures))


prefer to return this future directly rather than using promise.

In order to be consistent with other interfaces. And the upper layer also needs a future as return value.

yixinglu · 2022-03-10T11:13:32Z

tests/tck/features/mutate/ClearSpace.feature

+    And wait 2 seconds
+    When executing query:
+      """
+      CLEAR SPACE IF EXISTS clear_space;


Need you to consider the role of this operation? Could anyone clear the used space or only root user?

only root can do this. https://confluence.nebula-graph.io/pages/viewpage.action?pageId=40535032

Shylock-Hg · 2022-03-10T11:15:47Z

src/parser/scanner.lex

@@ -124,6 +124,7 @@ LABEL_FULL_WIDTH            {CN_EN_FULL_WIDTH}{CN_EN_NUM_FULL_WIDTH}*
 "ADD"                       { return TokenType::KW_ADD; }
 "CREATE"                    { return TokenType::KW_CREATE;}
 "DROP"                      { return TokenType::KW_DROP; }
+"CLEAR"                     { return TokenType::KW_CLEAR; } 


New reserved word means incompatible.

Weird. Doesn't the incompatibility mean that the this function affects other functions?

Means users maybe need to modify their application.

If the user does not use the clear space function, then there is no need to modify it. Add a new syntax, I think it is forward compatible.

When user has column of table named clear

yixinglu

LGTM, excellent!

panda-sheep · 2022-03-14T02:40:27Z

src/meta/processors/admin/ClearSpaceProcessor.cpp

+  }
+
+  // 5. Delete the space data on the corresponding hosts.
+  auto clearRet = adminClient_->clearSpace(spaceId, hosts).get();


When clearing data, other threads are writing data. At this time, how to ensure the correctness and consistency of data?

There will be a write prohibition command later, which will require the user to add a write prohibition command before executing clear.

Before clearing the space, do need to execute other SQL? Shouldn't it be triggered during clear space?

trigger together is not easy to guarantee atomicity, e.g. the meta server goes down when writes are disabled. When it starts up again, the write prohibition is still there. This increases the processing complexity. Therefore, this version is simple, let the user manually execute the prohibition command first.

👌， create snapshot has almost the same problem.

zhaohaifei added ready-for-testing PR: ready for the CI test doc affected PR: improvements or additions to documentation ready for review labels Mar 8, 2022

zhaohaifei added this to the v3.1.0 milestone Mar 8, 2022

zhaohaifei requested review from CPWstatic, dutor and a team as code owners March 8, 2022 09:38

zhaohaifei requested a review from critical27 March 8, 2022 09:41

critical27 reviewed Mar 8, 2022

View reviewed changes

zhaohaifei requested a review from critical27 March 9, 2022 08:38

zhaohaifei force-pushed the clear_space branch from 60bede3 to f394c20 Compare March 9, 2022 15:08

zhaohaifei added wip Solution: work in progress and removed ready-for-testing PR: ready for the CI test labels Mar 9, 2022

zhaohaifei changed the title ~~Clear space [part 1]~~ Clear space Mar 10, 2022

zhaohaifei added ready-for-testing PR: ready for the CI test and removed wip Solution: work in progress labels Mar 10, 2022

critical27 previously approved these changes Mar 10, 2022

View reviewed changes

yixinglu reviewed Mar 10, 2022

View reviewed changes

Shylock-Hg reviewed Mar 10, 2022

View reviewed changes

Shylock-Hg added the incompatible PR: incompatible with the recently released version label Mar 10, 2022

zhaohaifei dismissed critical27’s stale review via 98db6e7 March 11, 2022 02:32

zhaohaifei force-pushed the clear_space branch from 98db6e7 to 29d4c2e Compare March 11, 2022 02:33

zhaohaifei requested review from yixinglu, Shylock-Hg and critical27 March 11, 2022 05:00

yixinglu previously approved these changes Mar 14, 2022

View reviewed changes

critical27 previously approved these changes Mar 14, 2022

View reviewed changes

panda-sheep reviewed Mar 14, 2022

View reviewed changes

zhaohaifei added 15 commits March 14, 2022 13:34

Complete the code on the graph side

b4e2856

temp commit

f7530a2

requires careful review

1cf4fc0

complete the code

d56a52f

Compiled successfully

8fcd38d

add some comment

81178ac

fix review

cbc1ae3

temp commit

d122c11

temp commit

af4622c

add tck

347ff02

fix format

9b28469

fix review

2147298

add some tck

a7f29d9

fix tck

e5f13fd

fix comment

ebc089f

zhaohaifei dismissed stale reviews from critical27 and yixinglu via ebc089f March 14, 2022 05:34

zhaohaifei force-pushed the clear_space branch from 67e88ea to ebc089f Compare March 14, 2022 05:34

zhaohaifei requested review from panda-sheep and critical27 March 14, 2022 06:13

CPWstatic approved these changes Mar 14, 2022

View reviewed changes

yixinglu approved these changes Mar 14, 2022

View reviewed changes

yixinglu merged commit c9f046e into vesoft-inc:master Mar 14, 2022

zhaohaifei deleted the clear_space branch March 14, 2022 06:37

jamieliu1023 mentioned this pull request Mar 19, 2022

Weekly Report 2022-03-18 vesoft-inc/nebula-community#100

Closed

randomJoe211 mentioned this pull request Mar 21, 2022

Support the temporary lock up of a space to ensure that CLEAR SPACE and actually clear the space #4059

Open

Sophie-Xie mentioned this pull request May 6, 2022

Support to delete all data #3205

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clear space #3989

Clear space #3989

zhaohaifei commented Mar 8, 2022 •

edited by Sophie-Xie

Loading

Shylock-Hg commented Mar 8, 2022 •

edited

Loading

zhaohaifei commented Mar 8, 2022

critical27 Mar 8, 2022

critical27 Mar 8, 2022

critical27 Mar 8, 2022

critical27 Mar 8, 2022

critical27 Mar 8, 2022

critical27 left a comment

yixinglu Mar 10, 2022

yixinglu Mar 10, 2022

zhaohaifei Mar 11, 2022

yixinglu Mar 10, 2022

zhaohaifei Mar 11, 2022

Shylock-Hg Mar 10, 2022

zhaohaifei Mar 11, 2022

Shylock-Hg Mar 11, 2022 •

edited

Loading

zhaohaifei Mar 11, 2022

Shylock-Hg Mar 11, 2022

zhaohaifei Mar 11, 2022

yixinglu left a comment

panda-sheep Mar 14, 2022

zhaohaifei Mar 14, 2022

panda-sheep Mar 14, 2022

zhaohaifei Mar 14, 2022

panda-sheep Mar 14, 2022 •

edited

Loading

Clear space #3989

Clear space #3989

Conversation

zhaohaifei commented Mar 8, 2022 • edited by Sophie-Xie Loading

What type of PR is this?

What problem(s) does this PR solve?

Issue(s) number:

Description:

How do you solve it?

Special notes for your reviewer, ex. impact of this fix, design document, etc:

Checklist:

Release notes:

Shylock-Hg commented Mar 8, 2022 • edited Loading

zhaohaifei commented Mar 8, 2022

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

critical27 left a 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

Shylock-Hg Mar 11, 2022 • edited Loading

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

yixinglu left a 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

panda-sheep Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

zhaohaifei commented Mar 8, 2022 •

edited by Sophie-Xie

Loading

Shylock-Hg commented Mar 8, 2022 •

edited

Loading

Shylock-Hg Mar 11, 2022 •

edited

Loading

panda-sheep Mar 14, 2022 •

edited

Loading