acelyc111 commented on code in PR #1112:
URL: https://github.com/apache/incubator-pegasus/pull/1112#discussion_r948614410
##########
src/rdsn/src/common/backup_restore_common.cpp:
##########
@@ -36,5 +44,41 @@ const std::string
backup_restore_constant::BACKUP_ID("restore.backup_id");
const std::string
backup_restore_constant::SKIP_BAD_PARTITION("restore.skip_bad_partition");
const std::string
backup_restore_constant::RESTORE_PATH("restore.restore_path");
+std::string get_backup_root(const std::string &backup_root,
+ const std::string &user_defined_root_path)
+{
+ if (user_defined_root_path.empty()) {
+ return backup_root;
+ }
+ return utils::filesystem::path_combine(user_defined_root_path,
backup_root);
+}
+
+std::string get_backup_path(const std::string &root,
+ const std::string &app_name,
+ const int32_t app_id,
+ const int64_t backup_id,
+ const bool is_compatible)
+{
+ std::string str_app = app_name + "_" + std::to_string(app_id);
+ std::stringstream ss;
+ if (!is_compatible) {
+ ss << root << "/" << str_app << "/" << backup_id;
+ } else {
+ ss << root << "/" << backup_id << "/" << str_app;
+ }
+ return ss.str();
Review Comment:
how about use fmt::format instead?
##########
src/rdsn/src/common/backup_restore_common.h:
##########
@@ -52,5 +57,69 @@ class backup_restore_constant
static const std::string RESTORE_PATH;
};
+/// The directory structure on block service
+///
+/// (<root> = <user_define_root>/<cluster>)
+///
+/// <root>/<app_name>_<app_id>/<backup_id>/<pidx>/chkpt_<ip>_<port>/***.sst
+/// /<pidx>/chkpt_<ip>_<port>/CURRENT
+/// /<pidx>/chkpt_<ip>_<port>/IDENTITY
+/// /<pidx>/chkpt_<ip>_<port>/MANIFEST
+/// /<pidx>/chkpt_<ip>_<port>/OPTIONS
+/// /<pidx>/chkpt_<ip>_<port>/LOG
+///
/<pidx>/chkpt_<ip>_<port>/backup_metadata
+/// /<pidx>/current_checkpoint
+/// /<pidx>/data_version
+///
+/// ......other partitions......
+///
+/// <root>/<app_name>_<app_id>/<backup_id>/meta/app_metadata
+/// <root>/<app_name>_<app_id>/<backup_id>/backup_info
+///
+
+///
+/// The usage of files:
+/// 1, app_metadata : the metadata of the app, the same with the app's
app_info
+/// 2, backup_metadata : the file to statistic the information of a
checkpoint, include all the
+/// file's name, size and md5
+/// 3, current_checkpoint : specifing which checkpoint directory is valid
+/// 4, data_version: partition data_version
+/// 5, backup_info : recording the information of this backup
+
+// TODO(heyuchen): add other common functions
+// get_compatible_backup_root is only used for restore compatible backup
+
+// The backup root path on block service
+// if user_defined_root_path is not empty
+// - return <user_defined_root_path>/<backup_root>
+// else
+// - return <backup_root>
+std::string get_backup_root(const std::string &backup_root,
+ const std::string &user_defined_root_path);
+
+// This backup path on block service
+// if is_compatible = false (root is the return value of get_backup_root
function)
+// - return <root>/<app_name>_<app_id>/<backup_id>
+// else (only used for restore compatible backup, root is the return value of
+// get_compatible_backup_root function)
+// - return <root>/<backup_id>/<app_name>_<app_id>
+std::string get_backup_path(const std::string &root,
Review Comment:
if it is only used in backup_restore_common.cpp, how about define it as a
local anonymous function there?
##########
src/rdsn/src/common/backup_restore_common.cpp:
##########
@@ -36,5 +44,41 @@ const std::string
backup_restore_constant::BACKUP_ID("restore.backup_id");
const std::string
backup_restore_constant::SKIP_BAD_PARTITION("restore.skip_bad_partition");
const std::string
backup_restore_constant::RESTORE_PATH("restore.restore_path");
+std::string get_backup_root(const std::string &backup_root,
+ const std::string &user_defined_root_path)
+{
+ if (user_defined_root_path.empty()) {
+ return backup_root;
+ }
+ return utils::filesystem::path_combine(user_defined_root_path,
backup_root);
+}
+
+std::string get_backup_path(const std::string &root,
+ const std::string &app_name,
+ const int32_t app_id,
+ const int64_t backup_id,
+ const bool is_compatible)
+{
+ std::string str_app = app_name + "_" + std::to_string(app_id);
+ std::stringstream ss;
+ if (!is_compatible) {
+ ss << root << "/" << str_app << "/" << backup_id;
+ } else {
+ ss << root << "/" << backup_id << "/" << str_app;
+ }
+ return ss.str();
+}
+
+std::string get_backup_meta_path(const std::string &root,
+ const std::string &app_name,
+ const int32_t app_id,
+ const int64_t backup_id,
+ const bool is_compatible)
+{
+ std::stringstream ss;
+ ss << get_backup_path(root, app_name, app_id, backup_id, is_compatible) <<
"/meta";
Review Comment:
how about use fmt::format instead?
##########
src/rdsn/src/meta/test/meta_backup_engine_test.cpp:
##########
@@ -0,0 +1,107 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+#include <dsn/dist/fmt_logging.h>
+#include <dsn/utility/fail_point.h>
+
+#include "meta_test_base.h"
+#include "meta/meta_backup_engine.h"
+
+namespace dsn {
+namespace replication {
+
+class meta_backup_engine_test : public meta_test_base
+{
+public:
+ void SetUp()
+ {
+ meta_test_base::SetUp();
+ create_app(APP_NAME, PARTITION_COUNT);
+ std::shared_ptr<app_state> app = find_app(APP_NAME);
+ _app_id = app->app_id;
+ _backup_engine = std::make_shared<meta_backup_engine>(_ms.get(),
false);
+ fail::setup();
+ }
+
+ void TearDown()
Review Comment:
same
##########
src/rdsn/src/meta/test/meta_backup_engine_test.cpp:
##########
@@ -0,0 +1,107 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+#include <dsn/dist/fmt_logging.h>
+#include <dsn/utility/fail_point.h>
+
+#include "meta_test_base.h"
+#include "meta/meta_backup_engine.h"
+
+namespace dsn {
+namespace replication {
+
+class meta_backup_engine_test : public meta_test_base
+{
+public:
+ void SetUp()
Review Comment:
better to add `override`
##########
src/rdsn/src/meta/meta_backup_engine.h:
##########
@@ -92,12 +111,28 @@ class meta_backup_engine
void retry_backup(const dsn::gpid pid);
void handle_replica_backup_failed(const backup_response &response, const
gpid pid);
- error_code write_backup_file(const std::string &file_name, const dsn::blob
&write_buffer);
+ error_code write_backup_file(const std::string &remote_dir,
+ const std::string &file_name,
+ const blob &write_buffer);
error_code write_app_info();
void write_backup_info();
void update_backup_item_on_remote_storage(backup_status::type new_status,
int64_t end_time = 0);
+ std::string get_remote_storage_root() const
+ {
+ return
meta_options::concat_path_unix_style(_meta_svc->get_cluster_root(), "backup");
+ }
+
+ std::string get_remote_backup_path() const
+ {
+ auto type_path = _is_periodic_backup ? PERIODIC_PATH : ONETIME_PATH;
+ std::stringstream ss;
+ ss << get_remote_storage_root() << "/" << _cur_backup.app_id << "/" <<
type_path << "/"
+ << _cur_backup.backup_id;
+ return ss.str();
Review Comment:
how about use `fmt::format`?
##########
src/rdsn/src/meta/meta_backup_engine.h:
##########
@@ -40,6 +40,19 @@ struct app_backup_info
DEFINE_JSON_SERIALIZATION(backup_id, start_time_ms, end_time_ms, app_id,
app_name)
};
+///
+/// backup path on remote storage
+///
+/// Onetime backup:
+/// <cluster_root>/backup/<app_id>/once/<backup_id>/<backup_item>
+///
+/// Periodic backup:
+/// <cluster_root>/backup/<app_id>/periodic/<periodic_backup_policy>
+/// <cluster_root>/backup/<app_id>/periodic/<backup_id>/<backup_item>
+///
+static const std::string ONETIME_PATH = "once";
Review Comment:
Will these variables be used in some other files? if not, better to hide in
cpp.
##########
src/rdsn/src/common/backup_restore_common.h:
##########
@@ -52,5 +57,69 @@ class backup_restore_constant
static const std::string RESTORE_PATH;
};
+/// The directory structure on block service
+///
+/// (<root> = <user_define_root>/<cluster>)
+///
+/// <root>/<app_name>_<app_id>/<backup_id>/<pidx>/chkpt_<ip>_<port>/***.sst
+/// /<pidx>/chkpt_<ip>_<port>/CURRENT
+/// /<pidx>/chkpt_<ip>_<port>/IDENTITY
+/// /<pidx>/chkpt_<ip>_<port>/MANIFEST
+/// /<pidx>/chkpt_<ip>_<port>/OPTIONS
+/// /<pidx>/chkpt_<ip>_<port>/LOG
+///
/<pidx>/chkpt_<ip>_<port>/backup_metadata
+/// /<pidx>/current_checkpoint
+/// /<pidx>/data_version
+///
+/// ......other partitions......
+///
+/// <root>/<app_name>_<app_id>/<backup_id>/meta/app_metadata
+/// <root>/<app_name>_<app_id>/<backup_id>/backup_info
+///
+
+///
+/// The usage of files:
+/// 1, app_metadata : the metadata of the app, the same with the app's
app_info
+/// 2, backup_metadata : the file to statistic the information of a
checkpoint, include all the
+/// file's name, size and md5
+/// 3, current_checkpoint : specifing which checkpoint directory is valid
+/// 4, data_version: partition data_version
+/// 5, backup_info : recording the information of this backup
+
+// TODO(heyuchen): add other common functions
+// get_compatible_backup_root is only used for restore compatible backup
+
+// The backup root path on block service
+// if user_defined_root_path is not empty
+// - return <user_defined_root_path>/<backup_root>
+// else
+// - return <backup_root>
+std::string get_backup_root(const std::string &backup_root,
Review Comment:
where will this function been called? if only called in
src/rdsn/src/meta/meta_backup_engine.cpp, how about define it as a local
funciton there?
##########
src/rdsn/src/meta/meta_backup_engine.cpp:
##########
@@ -86,87 +86,57 @@ void meta_backup_engine::start()
// ThreadPool: THREAD_POOL_DEFAULT
error_code meta_backup_engine::write_app_info()
{
- // TODO(heyuchen): TBD
- return ERR_OK;
-}
-
-// ThreadPool: THREAD_POOL_DEFAULT
-void
meta_backup_engine::update_backup_item_on_remote_storage(backup_status::type
new_status,
- int64_t end_time)
-{
- // TODO(heyuchen): TBD
-}
-
-// TODO(heyuchen): update following functions
-
-error_code meta_backup_engine::write_backup_file(const std::string &file_name,
- const dsn::blob &write_buffer)
-{
- dist::block_service::create_file_request create_file_req;
- create_file_req.ignore_metadata = true;
- create_file_req.file_name = file_name;
-
- dsn::error_code err;
- dist::block_service::block_file_ptr remote_file;
- _block_service
- ->create_file(create_file_req,
- TASK_CODE_EXEC_INLINED,
- [&err, &remote_file](const
dist::block_service::create_file_response &resp) {
- err = resp.err;
- remote_file = resp.file_handle;
- })
- ->wait();
- if (err != dsn::ERR_OK) {
- ddebug_f("create file {} failed", file_name);
- return err;
- }
- dassert_f(remote_file != nullptr,
- "create file {} succeed, but can't get handle",
- create_file_req.file_name);
- remote_file
- ->write(dist::block_service::write_request{write_buffer},
- TASK_CODE_EXEC_INLINED,
- [&err](const dist::block_service::write_response &resp) { err
= resp.err; })
- ->wait();
- return err;
-}
-
-error_code meta_backup_engine::backup_app_meta()
-{
- dsn::blob app_info_buffer;
+ blob app_info_buffer;
{
zauto_read_lock l;
- _backup_service->get_state()->lock_read(l);
- std::shared_ptr<app_state> app =
_backup_service->get_state()->get_app(_cur_backup.app_id);
+ _meta_svc->get_server_state()->lock_read(l);
+ std::shared_ptr<app_state> app =
_meta_svc->get_server_state()->get_app(_cur_backup.app_id);
if (app == nullptr || app->status != app_status::AS_AVAILABLE) {
derror_f("app {} is not available, couldn't do backup now.",
_cur_backup.app_id);
return ERR_INVALID_STATE;
}
app_state tmp = *app;
- // Because we don't restore app envs, so no need to write app envs to
backup file.
- // TODO(zhangyifan): backup and restore app envs when needed.
- tmp.envs.clear();
- app_info_buffer = dsn::json::json_forwarder<app_info>::encode(tmp);
+ app_info_buffer = json::json_forwarder<app_info>::encode(tmp);
}
- std::string backup_root =
- dsn::utils::filesystem::path_combine(_backup_path,
_backup_service->backup_root());
- // TODO(heyuchen): refactor and update it in future
- std::string file_name = "todo";
- return write_backup_file(file_name, app_info_buffer);
+ std::string remote_meta_dir =
+ get_backup_meta_path(get_backup_root(FLAGS_cold_backup_root,
_cur_backup.backup_path),
Review Comment:
can we move `get_backup_root` into `get_backup_meta_path`, and only left
`_cur_backup.backup_path` here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
