[GitHub] [incubator-pegasus] foreverneverer commented on a diff in pull request #1102: feat(backup): 2. update and refactor meta backup engine class

[GitBox <gi...@apache.org>]

foreverneverer commented on code in PR #1102:
URL: https://github.com/apache/incubator-pegasus/pull/1102#discussion_r941012247


##########
src/rdsn/src/meta/meta_backup_engine.cpp:
##########
@@ -16,74 +16,93 @@
 // under the License.
 
 #include <dsn/dist/fmt_logging.h>
+#include <dsn/utility/fail_point.h>
 #include <dsn/utility/filesystem.h>
 
-#include "common/backup_common.h"
-#include "common/replication_common.h"
-#include "server_state.h"
+#include "meta_backup_engine.h"
 
 namespace dsn {
 namespace replication {
 
-backup_engine::backup_engine(backup_service *service)
-    : _backup_service(service), _block_service(nullptr), _backup_path(""), _is_backup_failed(false)
+meta_backup_engine::meta_backup_engine(meta_service *meta_svc) : _meta_svc(meta_svc) {}
+
+meta_backup_engine::meta_backup_engine(meta_service *meta_svc, bool is_periodic)
+    : _meta_svc(meta_svc), _is_periodic_backup(is_periodic)
 {
 }
 
-backup_engine::~backup_engine() { _tracker.cancel_outstanding_tasks(); }
+meta_backup_engine::~meta_backup_engine() { _tracker.cancel_outstanding_tasks(); }
 
-error_code backup_engine::init_backup(int32_t app_id)
+// ThreadPool: THREAD_POOL_DEFAULT
+void meta_backup_engine::init_backup(int32_t app_id,
+                                     int32_t partition_count,
+                                     const std::string &app_name,
+                                     const std::string &provider,
+                                     const std::string &backup_root_path)
 {
-    std::string app_name;
-    int partition_count;
-    {
-        zauto_read_lock l;
-        _backup_service->get_state()->lock_read(l);
-        std::shared_ptr<app_state> app = _backup_service->get_state()->get_app(app_id);
-        if (app == nullptr || app->status != app_status::AS_AVAILABLE) {
-            derror_f("app {} is not available, couldn't do backup now.", app_id);
-            return ERR_INVALID_STATE;
-        }
-        app_name = app->app_name;
-        partition_count = app->partition_count;
-    }
-
-    zauto_lock lock(_lock);
+    zauto_write_lock l(_lock);
     _backup_status.clear();
     for (int i = 0; i < partition_count; ++i) {
-        _backup_status.emplace(i, backup_status::UNALIVE);
+        _backup_status.emplace(i, backup_status::INVALID);
     }
     _cur_backup.app_id = app_id;
     _cur_backup.app_name = app_name;
     _cur_backup.backup_id = static_cast<int64_t>(dsn_now_ms());
     _cur_backup.start_time_ms = _cur_backup.backup_id;
-    return ERR_OK;
+    _cur_backup.backup_provider_type = provider;
+    _cur_backup.backup_path = backup_root_path;
+    _cur_backup.status = backup_status::INVALID;
+    _is_backup_failed = false;
+    _is_backup_canceled = false;
 }
 
-error_code backup_engine::set_block_service(const std::string &provider)
+// ThreadPool: THREAD_POOL_DEFAULT
+void meta_backup_engine::start()
 {
-    _provider_type = provider;
-    _block_service = _backup_service->get_meta_service()
-                         ->get_block_service_manager()
-                         .get_or_create_block_filesystem(provider);
-    if (_block_service == nullptr) {
-        return ERR_INVALID_PARAMETERS;
+    ddebug_f("App[{}] start {} backup[{}] on {}, root_path = {}",
+             _cur_backup.app_name,
+             _is_periodic_backup ? "periodic" : "onetime",
+             _cur_backup.backup_id,
+             _cur_backup.backup_provider_type,
+             _cur_backup.backup_path);
+    error_code err = write_app_info();
+    if (err != ERR_OK) {
+        derror_f("backup_id({}): backup meta data for app {} failed, error {}",
+                 _cur_backup.backup_id,
+                 _cur_backup.app_id,
+                 err);
+        update_backup_item_on_remote_storage(backup_status::FAILED, dsn_now_ms());
+        return;
+    }
+    update_backup_item_on_remote_storage(backup_status::CHECKPOINTING);

Review Comment:
   do we still storage the status on zk?



##########
src/rdsn/src/common/backup.thrift:
##########
@@ -20,6 +20,30 @@ include "../../../../idl/dsn.layer2.thrift"
 
 namespace cpp dsn.replication
 
+enum backup_status
+{
+    INVALID,
+    CHECKPOINTING,
+    CHECKPOINTED,
+    UPLOADING,

Review Comment:
   if it has no other logic between `CHECKPOINTED` and `UPLOADING`, I think delete `CHECKPOINTED` is ok. checkpoint complete also mean it step to uploading



-- 
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: dev-unsubscribe@pegasus.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org