sst download/ingest #2265

xuguruogu · 2020-08-02T16:33:48Z

To work with https://github.com/xuguruogu/nebula-sst.

Use sub dir for every partition for SST files.
Can ingest tag/edge only, support download/ingest concurrently by using sub dir for every tag/edge type.

darionyaphet · 2020-08-03T06:04:05Z

src/common/hdfs/HdfsHelper.h

@@ -26,6 +26,15 @@ class HdfsHelper {
                                              const std::string& hdfsPath,
                                              const std::string& localPath) = 0;

+    virtual StatusOr<bool> exist(const std::string& hdfsHost,


So you don't implement this function?

Implement it in another class.

why don't define it as a pure virtual function ?

darionyaphet · 2020-08-03T06:06:16Z

src/kvstore/RocksEngineConfig.cpp

@@ -171,7 +171,7 @@ rocksdb::Status initRocksdbOptions(rocksdb::Options &baseOpts) {
        bbtOpts.no_block_cache = true;
    } else {
        static std::shared_ptr<rocksdb::Cache> blockCache
-            = rocksdb::NewLRUCache(FLAGS_rocksdb_block_cache * 1024 * 1024, 8/*shard bits*/);
+            = rocksdb::NewLRUCache(FLAGS_rocksdb_block_cache * 1024 * 1024, 12/*shard bits*/);


why modify the default value ?

2^8 = 256 shards seem a little bit small for modern machines with nearly 100 CPUs.

keep unchanged.

darionyaphet · 2020-08-03T06:08:13Z

src/parser/MutateSentences.cpp

+    if (tag()) {
+        buf += "tag ";
+        buf += *tag();
+        buf += " ";


you can move the space before HDFS

darionyaphet · 2020-08-03T06:08:22Z

src/parser/MutateSentences.cpp

+    if (tag()) {
+        buf += "tag ";
+        buf += *tag();
+        buf += " ";


darionyaphet · 2020-08-03T06:11:19Z

src/common/hdfs/test/HdfsHelperTest.cpp

-    auto helper = HdfsCommandHelper();
-    auto result = helper.ls("127.0.0.1", 9000, "/");
-    ASSERT_TRUE(result.ok()) << result.status();
+//    auto helper = HdfsCommandHelper();


why cancel this test case ?

It should never pass.

maybe you add some check about the result code Could you remove it?

In fact, it should fail if can not connect to hdfs

Please remove the code if useless.

Please fix the comment

src/meta/MetaHttpDownloadHandler.cpp

darionyaphet · 2020-08-04T02:53:14Z

src/common/hdfs/HdfsCommandHelper.cpp

    }
 }

 StatusOr<std::string> HdfsCommandHelper::copyToLocal(const std::string& hdfsHost,
                                                     int32_t hdfsPort,
                                                     const std::string& hdfsPath,
                                                     const std::string& localPath) {
-    auto command = folly::stringPrintf("hdfs dfs -copyToLocal hdfs://%s:%d%s %s",


there are any different between hdfs dfs and hadoop fs ?

The later is the recommended usage in the newer version.

darionyaphet · 2020-08-04T03:29:22Z

src/meta/MetaHttpDownloadHandler.cpp

@@ -87,7 +106,7 @@ void MetaHttpDownloadHandler::onEOM() noexcept {
    }

    if (helper_->checkHadoopPath()) {
-        if (dispatchSSTFiles(hdfsHost_, hdfsPort_, hdfsPath_)) {


darionyaphet · 2020-08-04T03:40:45Z

src/common/hdfs/HdfsCommandHelper.cpp

+                                        int32_t hdfsPort,
+                                        const std::string& hdfsPath) {
+    auto path = folly::stringPrintf("hdfs://%s:%d%s", hdfsHost.c_str(), hdfsPort, hdfsPath.c_str());
+    auto command = folly::stringPrintf("hadoop fs -test -e %s", path.c_str());


should support -d to check it is a directory?

darionyaphet · 2020-08-04T03:42:55Z

src/kvstore/RocksEngine.cpp

@@ -340,6 +357,12 @@ int32_t RocksEngine::totalPartsNum() {

 ResultCode RocksEngine::ingest(const std::vector<std::string>& files) {
    rocksdb::IngestExternalFileOptions options;
+    options.move_files = true;


If you set verify_checksums_before_ingest, would you set verify_checksums_readahead_size to improve the performance?

dangleptr · 2020-08-05T08:46:32Z

src/kvstore/NebulaStore.cpp

+                continue;
+            }
+
+            auto code = engine->ingest(files);


Currently, how many SST files generated for one part?

It can be configured in spark sst generater side. we use 100 at most, for each sst file 800MB

Could you ensure they are global in order?

dangleptr · 2020-08-05T08:59:01Z

src/meta/MetaHttpDownloadHandler.cpp

+    auto existStatus = helper_->exist(hdfsHost_, hdfsPort_, hdfsPath_);
+    if (!existStatus.ok()) {
+        LOG(ERROR) << "Run Hdfs Test failed. hdfs://" << hdfsHost_ << ":" << hdfsPort_ << hdfsPath_;
+        err_ = HttpCode::E_ILLEGAL_ARGUMENT;


dangleptr · 2020-08-05T09:01:26Z

src/storage/http/StorageHttpDownloadHandler.cpp

+    }
+    if (headers->hasQueryParam("tag")) {
+        auto& tag = headers->getQueryParam("tag");
+        tag_.assign(folly::to<TagID>(tag));


folly::to maybe throw exceptions, we should catch them, or use tagName directly.

Please fix the comments

dangleptr · 2020-08-05T09:01:32Z

src/storage/http/StorageHttpDownloadHandler.cpp

+    }
+    if (headers->hasQueryParam("edge")) {
+        auto& edge = headers->getQueryParam("edge");
+        edge_.assign(folly::to<EdgeType>(edge));


dangleptr · 2020-08-05T09:02:13Z

src/storage/http/StorageHttpDownloadHandler.cpp

+        std::string downloadRootPathEdge = downloadRootPath + "/edge";
+        std::string downloadRootPathTag = downloadRootPath + "/tag";
+        std::string downloadRootPathGeneral = downloadRootPath + "/general";
+        _clean_subdirs(downloadRootPathEdge);


Please follow our code style for the function name.

dangleptr · 2020-08-05T09:03:46Z

src/storage/http/StorageHttpDownloadHandler.cpp

-                                                  const std::string& hdfsPath,
-                                                  const std::vector<std::string>& parts) {
-    static std::atomic_flag isRunning = ATOMIC_FLAG_INIT;
-    if (isRunning.test_and_set()) {


why remove it?

concurrently download is needed, with different tag/edge in different sub dirs.

Make sense.

dangleptr · 2020-08-05T09:05:29Z

src/storage/http/StorageHttpDownloadHandler.cpp

+        std::string localPath;
+        if (edge_.has_value()) {
+            localPath = folly::stringPrintf(
+                "%s/download/edge/%d", partDataRoot, edge_.value());


You put all parts data together?

// output dir: // -- 1/1-1.sst // /1-2.sst // /1-3.sst // -- 2/2-1.sst // /2-2.sst

dangleptr · 2020-08-05T09:05:51Z

src/storage/http/StorageHttpIngestHandler.cpp

+    spaceID_ = headers->getIntQueryParam("space");
+    if (headers->hasQueryParam("tag")) {
+        auto& tag = headers->getQueryParam("tag");
+        tag_.assign(folly::to<TagID>(tag));


Please fix the comments

dangleptr · 2020-08-05T09:05:58Z

src/storage/http/StorageHttpIngestHandler.cpp

+    }
+    if (headers->hasQueryParam("edge")) {
+        auto& edge = headers->getQueryParam("edge");
+        edge_.assign(folly::to<EdgeType>(edge));


dangleptr · 2020-08-05T09:08:25Z

src/common/process/ProcessUtils.cpp

@@ -142,9 +142,12 @@ pid_t ProcessUtils::maxPid() {


 StatusOr<std::string> ProcessUtils::runCommand(const char* command) {
+    std::string cmd;
+    cmd += command;
+    cmd += " 2>&1";


why put it in command by default. I don't think it is really a good idea.

With checking exit code, it can be removed.

dangleptr · 2020-08-06T04:08:00Z

src/storage/http/StorageHttpDownloadHandler.cpp

@@ -18,6 +18,8 @@

 DEFINE_int32(download_thread_num, 3, "download thread number");

+DEFINE_int32(download_keep_day, 1, "download keep days");


What's the purpose about this flag. Why not delete them directly.

The SST files are moved to rocksdb. The flag is used to handle some SST files left for some unknown reasons, e.g. OOM.

So how to handle the cases just partial SST files ingested successfully.

delete them after some days when download invoked

dangleptr · 2020-08-06T06:12:21Z

src/common/hdfs/test/HdfsHelperTest.cpp

-    auto helper = HdfsCommandHelper();
-    auto result = helper.ls("127.0.0.1", 9000, "/");
-    ASSERT_TRUE(result.ok()) << result.status();
+//    auto helper = HdfsCommandHelper();


Please remove the code if useless.

dangleptr · 2020-08-10T09:28:38Z

Please check your code style.

dangleptr

Thanks for your contribution.
The pr looks good to me now.

darionyaphet

LGTM thanks

Co-authored-by: trippli <trippli@tencent.com> Co-authored-by: dangleptr <37216992+dangleptr@users.noreply.github.com> Co-authored-by: yaphet <darion.wang@vesoft.com>

xuguruogu force-pushed the sst branch from d8d6fc0 to d6b0b2a Compare August 2, 2020 16:41

jude-zhu requested review from dangleptr, dutor and darionyaphet August 3, 2020 01:18

xuguruogu force-pushed the sst branch from d6b0b2a to f6bc247 Compare August 3, 2020 04:33

darionyaphet reviewed Aug 3, 2020

View reviewed changes

src/parser/MutateSentences.cpp Outdated

if (tag()) {

buf += "tag ";

buf += *tag();

buf += " ";

Copy link

Contributor

darionyaphet Aug 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

darionyaphet reviewed Aug 3, 2020

View reviewed changes

src/meta/MetaHttpDownloadHandler.cpp Outdated Show resolved Hide resolved

xuguruogu force-pushed the sst branch from f6bc247 to 2cdae4d Compare August 3, 2020 07:37

darionyaphet reviewed Aug 4, 2020

View reviewed changes

xuguruogu force-pushed the sst branch 3 times, most recently from 584aedd to f4a8e5d Compare August 4, 2020 11:00

jude-zhu requested a review from darionyaphet August 5, 2020 08:42

dangleptr reviewed Aug 5, 2020

View reviewed changes

xuguruogu force-pushed the sst branch from f4a8e5d to 9dd67d0 Compare August 5, 2020 09:27

jude-zhu requested a review from dangleptr August 6, 2020 03:32

dangleptr reviewed Aug 6, 2020

View reviewed changes

xuguruogu force-pushed the sst branch from 9dd67d0 to 65966ff Compare August 7, 2020 06:12

xuguruogu requested a review from dangleptr August 10, 2020 07:35

xuguruogu force-pushed the sst branch 2 times, most recently from 5549867 to a60be82 Compare August 10, 2020 08:59

dangleptr added the ready-for-testing PR: ready for the CI test label Aug 10, 2020

xuguruogu force-pushed the sst branch from a60be82 to 6e62fca Compare August 10, 2020 10:00

sst download/ingest

feb3929

xuguruogu force-pushed the sst branch from 6e62fca to feb3929 Compare August 10, 2020 11:19

Merge branch 'master' into sst

28f5555

dangleptr approved these changes Aug 11, 2020

View reviewed changes

darionyaphet approved these changes Aug 11, 2020

View reviewed changes

Merge branch 'master' into sst

844e01b

dangleptr merged commit 0e2aa4a into vesoft-inc:master Aug 11, 2020

		@@ -18,6 +18,8 @@

		DEFINE_int32(download_thread_num, 3, "download thread number");

		DEFINE_int32(download_keep_day, 1, "download keep days");

sst download/ingest #2265

sst download/ingest #2265

Conversation

xuguruogu commented Aug 2, 2020 • 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

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

Choose a reason for hiding this comment

dangleptr Aug 5, 2020 • 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

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

xuguruogu Aug 5, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dangleptr commented Aug 10, 2020

dangleptr left a comment

Choose a reason for hiding this comment

darionyaphet left a comment

Choose a reason for hiding this comment

xuguruogu commented Aug 2, 2020 •

edited

Loading

dangleptr Aug 5, 2020 •

edited

Loading

xuguruogu Aug 5, 2020 •

edited

Loading