You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by GitBox <gi...@apache.org> on 2022/09/07 02:31:30 UTC
[GitHub] [incubator-pegasus] hycdong opened a new pull request, #1150: feat(backup): 9. replica handle backup clear context and invalid status
hycdong opened a new pull request, #1150:
URL: https://github.com/apache/incubator-pegasus/pull/1150
https://github.com/apache/incubator-pegasus/issues/1081
--
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
[GitHub] [incubator-pegasus] hycdong commented on a diff in pull request #1150: feat(backup): 9. replica handle backup clear context and invalid status
Posted by GitBox <gi...@apache.org>.
hycdong commented on code in PR #1150:
URL: https://github.com/apache/incubator-pegasus/pull/1150#discussion_r976090132
##########
docker/README.md:
##########
@@ -27,7 +27,7 @@ to deploying a standalone cluster of Pegasus containers on your local machine.
[](https://github.com/apache/incubator-pegasus/actions/workflows/thirdparty-regular-push.yml)
-[](https://github.com/apache/incubator-pegasus/actions/workflows/pegasus-regular-build.yml)
+[](https://github.com/apache/incubator-pegasus/actions/workflows/regular-build.yml)
Review Comment:
Sure, I knew that, but I plan to rebase master once when I finish all prs and try to merge backup_restore_dev into master :-)
--
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
[GitHub] [incubator-pegasus] hycdong commented on a diff in pull request #1150: feat(backup): 9. replica handle backup clear context and invalid status
Posted by GitBox <gi...@apache.org>.
hycdong commented on code in PR #1150:
URL: https://github.com/apache/incubator-pegasus/pull/1150#discussion_r975975604
##########
src/rdsn/src/replica/backup/test/replica_backup_manager_test.cpp:
##########
@@ -120,7 +143,178 @@ class replica_backup_manager_test : public replica_test_base
std::unique_ptr<replica_backup_manager> _backup_mgr;
};
-// TODO(heyuchen): add unit test for on_backup after implement all status
+TEST_F(replica_backup_manager_test, on_backup_test)
+{
+ fail::cfg("replica_backup_start_checkpointing", "return()");
+ fail::cfg("replica_backup_start_uploading", "return()");
+ fail::cfg("replica_backup_upload_completed", "return()");
+ fail::cfg("replica_backup_clear_context", "return()");
+
+ struct test_struct
+ {
+ backup_status::type local_status;
+ backup_status::type request_status;
+ error_code expected_err;
+ backup_status::type expected_status;
+ } tests[]{
+ // request_status = UNINITIALIZED
+ {backup_status::UNINITIALIZED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::SUCCEED},
+ {backup_status::FAILED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = CHECKPOINTING
+ {backup_status::UNINITIALIZED,
+ backup_status::CHECKPOINTING,
+ ERR_OK,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTING,
+ backup_status::CHECKPOINTING,
+ ERR_OK,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::CHECKPOINTING,
+ ERR_OK,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::SUCCEED},
+ {backup_status::FAILED,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = CHECKPOINTED
+ {backup_status::UNINITIALIZED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::SUCCEED},
+ {backup_status::FAILED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = UPLOADING
+ {backup_status::UNINITIALIZED,
+ backup_status::UPLOADING,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::UPLOADING,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED, backup_status::UPLOADING, ERR_OK, backup_status::UPLOADING},
+ {backup_status::UPLOADING, backup_status::UPLOADING, ERR_OK, backup_status::UPLOADING},
+ {backup_status::SUCCEED, backup_status::UPLOADING, ERR_OK, backup_status::SUCCEED},
+ {backup_status::FAILED, backup_status::UPLOADING, ERR_INVALID_STATE, backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::UPLOADING,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = SUCCEED
+ {backup_status::UNINITIALIZED,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED, backup_status::SUCCEED, ERR_INVALID_STATE, backup_status::SUCCEED},
+ {backup_status::FAILED, backup_status::SUCCEED, ERR_INVALID_STATE, backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = FAILED
+ {backup_status::UNINITIALIZED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::UPLOADING, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::SUCCEED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::FAILED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CANCELED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ // request_status = CANCELED
+ {backup_status::UNINITIALIZED,
+ backup_status::CANCELED,
+ ERR_OK,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::CANCELED,
+ ERR_OK,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTED,
+ backup_status::CANCELED,
+ ERR_OK,
+ backup_status::UNINITIALIZED},
+ {backup_status::UPLOADING, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::SUCCEED, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::FAILED, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CANCELED, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED}};
+
+ for (const auto &test : tests) {
+ ASSERT_EQ(on_backup(test.local_status, test.request_status), test.expected_err);
Review Comment:
I think it is okay to remain old implementation. For developer, it is easy to judge which test failed according to the unit test logs, and I think old implemenation is more graceful.
--
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
[GitHub] [incubator-pegasus] hycdong commented on a diff in pull request #1150: feat(backup): 9. replica handle backup clear context and invalid status
Posted by GitBox <gi...@apache.org>.
hycdong commented on code in PR #1150:
URL: https://github.com/apache/incubator-pegasus/pull/1150#discussion_r976082780
##########
src/rdsn/src/common/backup_restore_common.h:
##########
@@ -29,6 +29,7 @@ namespace dsn {
namespace replication {
DSN_DECLARE_string(cold_backup_root);
+DSN_DECLARE_uint32(cold_backup_checkpoint_reserve_minutes);
Review Comment:
Maybe other configuration can declare in cpp file, but as my comment shows, this config `cold_backup_checkpoint_reserve_minutes` will be used in different cpp files, it need to be defined in header file.
--
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
[GitHub] [incubator-pegasus] hycdong commented on a diff in pull request #1150: feat(backup): 9. replica handle backup clear context and invalid status
Posted by GitBox <gi...@apache.org>.
hycdong commented on code in PR #1150:
URL: https://github.com/apache/incubator-pegasus/pull/1150#discussion_r976095026
##########
src/rdsn/src/common/backup_restore_common.h:
##########
@@ -29,6 +29,7 @@ namespace dsn {
namespace replication {
DSN_DECLARE_string(cold_backup_root);
+DSN_DECLARE_uint32(cold_backup_checkpoint_reserve_minutes);
Review Comment:
In my memory, it will be used in at least 3 files. Will declaring it in different 3 cpp files is more effecitive than declaring it in one header file? I just don't know which way is more effective.
--
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
[GitHub] [incubator-pegasus] hycdong commented on a diff in pull request #1150: feat(backup): 9. replica handle backup clear context and invalid status
Posted by GitBox <gi...@apache.org>.
hycdong commented on code in PR #1150:
URL: https://github.com/apache/incubator-pegasus/pull/1150#discussion_r976095026
##########
src/rdsn/src/common/backup_restore_common.h:
##########
@@ -29,6 +29,7 @@ namespace dsn {
namespace replication {
DSN_DECLARE_string(cold_backup_root);
+DSN_DECLARE_uint32(cold_backup_checkpoint_reserve_minutes);
Review Comment:
In my memory, it will be used in at least 3 cpp files in future pr. Will declaring it in different 3 cpp files is more effecitive than declaring it in one header file? I just don't know which way is more effective.
--
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
[GitHub] [incubator-pegasus] hycdong commented on a diff in pull request #1150: feat(backup): 9. replica handle backup clear context and invalid status
Posted by GitBox <gi...@apache.org>.
hycdong commented on code in PR #1150:
URL: https://github.com/apache/incubator-pegasus/pull/1150#discussion_r976340345
##########
src/rdsn/src/common/backup_restore_common.h:
##########
@@ -29,6 +29,7 @@ namespace dsn {
namespace replication {
DSN_DECLARE_string(cold_backup_root);
+DSN_DECLARE_uint32(cold_backup_checkpoint_reserve_minutes);
Review Comment:
Got it~ Updated :-)
--
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
[GitHub] [incubator-pegasus] hycdong commented on a diff in pull request #1150: feat(backup): 9. replica handle backup clear context and invalid status
Posted by GitBox <gi...@apache.org>.
hycdong commented on code in PR #1150:
URL: https://github.com/apache/incubator-pegasus/pull/1150#discussion_r975974515
##########
src/rdsn/src/common/backup_restore_common.h:
##########
@@ -29,6 +29,7 @@ namespace dsn {
namespace replication {
DSN_DECLARE_string(cold_backup_root);
+DSN_DECLARE_uint32(cold_backup_checkpoint_reserve_minutes);
Review Comment:
It will be used in different cpp files, so I think it need to be defined in header file.
--
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
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1150: feat(backup): 9. replica handle backup clear context and invalid status
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1150:
URL: https://github.com/apache/incubator-pegasus/pull/1150#discussion_r976025628
##########
src/rdsn/src/replica/backup/test/replica_backup_manager_test.cpp:
##########
@@ -120,7 +143,178 @@ class replica_backup_manager_test : public replica_test_base
std::unique_ptr<replica_backup_manager> _backup_mgr;
};
-// TODO(heyuchen): add unit test for on_backup after implement all status
+TEST_F(replica_backup_manager_test, on_backup_test)
+{
+ fail::cfg("replica_backup_start_checkpointing", "return()");
+ fail::cfg("replica_backup_start_uploading", "return()");
+ fail::cfg("replica_backup_upload_completed", "return()");
+ fail::cfg("replica_backup_clear_context", "return()");
+
+ struct test_struct
+ {
+ backup_status::type local_status;
+ backup_status::type request_status;
+ error_code expected_err;
+ backup_status::type expected_status;
+ } tests[]{
+ // request_status = UNINITIALIZED
+ {backup_status::UNINITIALIZED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::SUCCEED},
+ {backup_status::FAILED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = CHECKPOINTING
+ {backup_status::UNINITIALIZED,
+ backup_status::CHECKPOINTING,
+ ERR_OK,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTING,
+ backup_status::CHECKPOINTING,
+ ERR_OK,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::CHECKPOINTING,
+ ERR_OK,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::SUCCEED},
+ {backup_status::FAILED,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = CHECKPOINTED
+ {backup_status::UNINITIALIZED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::SUCCEED},
+ {backup_status::FAILED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = UPLOADING
+ {backup_status::UNINITIALIZED,
+ backup_status::UPLOADING,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::UPLOADING,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED, backup_status::UPLOADING, ERR_OK, backup_status::UPLOADING},
+ {backup_status::UPLOADING, backup_status::UPLOADING, ERR_OK, backup_status::UPLOADING},
+ {backup_status::SUCCEED, backup_status::UPLOADING, ERR_OK, backup_status::SUCCEED},
+ {backup_status::FAILED, backup_status::UPLOADING, ERR_INVALID_STATE, backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::UPLOADING,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = SUCCEED
+ {backup_status::UNINITIALIZED,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED, backup_status::SUCCEED, ERR_INVALID_STATE, backup_status::SUCCEED},
+ {backup_status::FAILED, backup_status::SUCCEED, ERR_INVALID_STATE, backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = FAILED
+ {backup_status::UNINITIALIZED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::UPLOADING, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::SUCCEED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::FAILED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CANCELED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ // request_status = CANCELED
+ {backup_status::UNINITIALIZED,
+ backup_status::CANCELED,
+ ERR_OK,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::CANCELED,
+ ERR_OK,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTED,
+ backup_status::CANCELED,
+ ERR_OK,
+ backup_status::UNINITIALIZED},
+ {backup_status::UPLOADING, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::SUCCEED, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::FAILED, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CANCELED, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED}};
+
+ for (const auto &test : tests) {
+ ASSERT_EQ(on_backup(test.local_status, test.request_status), test.expected_err);
Review Comment:
There are so many test cases, is there a convenient way to distinguish which one failed?
--
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
[GitHub] [incubator-pegasus] acelyc111 merged pull request #1150: feat(backup): 9. replica handle backup clear context and invalid status
Posted by GitBox <gi...@apache.org>.
acelyc111 merged PR #1150:
URL: https://github.com/apache/incubator-pegasus/pull/1150
--
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
[GitHub] [incubator-pegasus] hycdong commented on a diff in pull request #1150: feat(backup): 9. replica handle backup clear context and invalid status
Posted by GitBox <gi...@apache.org>.
hycdong commented on code in PR #1150:
URL: https://github.com/apache/incubator-pegasus/pull/1150#discussion_r976084409
##########
src/rdsn/src/replica/backup/test/replica_backup_manager_test.cpp:
##########
@@ -120,7 +143,178 @@ class replica_backup_manager_test : public replica_test_base
std::unique_ptr<replica_backup_manager> _backup_mgr;
};
-// TODO(heyuchen): add unit test for on_backup after implement all status
+TEST_F(replica_backup_manager_test, on_backup_test)
+{
+ fail::cfg("replica_backup_start_checkpointing", "return()");
+ fail::cfg("replica_backup_start_uploading", "return()");
+ fail::cfg("replica_backup_upload_completed", "return()");
+ fail::cfg("replica_backup_clear_context", "return()");
+
+ struct test_struct
+ {
+ backup_status::type local_status;
+ backup_status::type request_status;
+ error_code expected_err;
+ backup_status::type expected_status;
+ } tests[]{
+ // request_status = UNINITIALIZED
+ {backup_status::UNINITIALIZED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::SUCCEED},
+ {backup_status::FAILED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = CHECKPOINTING
+ {backup_status::UNINITIALIZED,
+ backup_status::CHECKPOINTING,
+ ERR_OK,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTING,
+ backup_status::CHECKPOINTING,
+ ERR_OK,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::CHECKPOINTING,
+ ERR_OK,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::SUCCEED},
+ {backup_status::FAILED,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = CHECKPOINTED
+ {backup_status::UNINITIALIZED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::SUCCEED},
+ {backup_status::FAILED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = UPLOADING
+ {backup_status::UNINITIALIZED,
+ backup_status::UPLOADING,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::UPLOADING,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED, backup_status::UPLOADING, ERR_OK, backup_status::UPLOADING},
+ {backup_status::UPLOADING, backup_status::UPLOADING, ERR_OK, backup_status::UPLOADING},
+ {backup_status::SUCCEED, backup_status::UPLOADING, ERR_OK, backup_status::SUCCEED},
+ {backup_status::FAILED, backup_status::UPLOADING, ERR_INVALID_STATE, backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::UPLOADING,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = SUCCEED
+ {backup_status::UNINITIALIZED,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED, backup_status::SUCCEED, ERR_INVALID_STATE, backup_status::SUCCEED},
+ {backup_status::FAILED, backup_status::SUCCEED, ERR_INVALID_STATE, backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = FAILED
+ {backup_status::UNINITIALIZED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::UPLOADING, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::SUCCEED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::FAILED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CANCELED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ // request_status = CANCELED
+ {backup_status::UNINITIALIZED,
+ backup_status::CANCELED,
+ ERR_OK,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::CANCELED,
+ ERR_OK,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTED,
+ backup_status::CANCELED,
+ ERR_OK,
+ backup_status::UNINITIALIZED},
+ {backup_status::UPLOADING, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::SUCCEED, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::FAILED, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CANCELED, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED}};
+
+ for (const auto &test : tests) {
+ ASSERT_EQ(on_backup(test.local_status, test.request_status), test.expected_err);
Review Comment:
I have to admit that it is only be shown by unit test log.
--
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
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1150: feat(backup): 9. replica handle backup clear context and invalid status
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1150:
URL: https://github.com/apache/incubator-pegasus/pull/1150#discussion_r976089172
##########
src/rdsn/src/common/backup_restore_common.h:
##########
@@ -29,6 +29,7 @@ namespace dsn {
namespace replication {
DSN_DECLARE_string(cold_backup_root);
+DSN_DECLARE_uint32(cold_backup_checkpoint_reserve_minutes);
Review Comment:
If I disn't miss anything, ther are only 2 files use it, delare it in thw 2 files will cause any problem?
##########
docker/README.md:
##########
@@ -27,7 +27,7 @@ to deploying a standalone cluster of Pegasus containers on your local machine.
[](https://github.com/apache/incubator-pegasus/actions/workflows/thirdparty-regular-push.yml)
-[](https://github.com/apache/incubator-pegasus/actions/workflows/pegasus-regular-build.yml)
+[](https://github.com/apache/incubator-pegasus/actions/workflows/regular-build.yml)
Review Comment:
I missed that this is on a develop branch, ignore that.
##########
src/rdsn/src/common/backup_restore_common.h:
##########
@@ -29,6 +29,7 @@ namespace dsn {
namespace replication {
DSN_DECLARE_string(cold_backup_root);
+DSN_DECLARE_uint32(cold_backup_checkpoint_reserve_minutes);
Review Comment:
If I didn't miss anything, ther are only 2 files use it, delare it in thw 2 files will cause any problem?
--
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
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1150: feat(backup): 9. replica handle backup clear context and invalid status
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1150:
URL: https://github.com/apache/incubator-pegasus/pull/1150#discussion_r976089172
##########
src/rdsn/src/common/backup_restore_common.h:
##########
@@ -29,6 +29,7 @@ namespace dsn {
namespace replication {
DSN_DECLARE_string(cold_backup_root);
+DSN_DECLARE_uint32(cold_backup_checkpoint_reserve_minutes);
Review Comment:
If I didn't miss anything, ther are only 2 files use it, delare it in the 2 files will cause any problem?
--
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
[GitHub] [incubator-pegasus] hycdong commented on a diff in pull request #1150: feat(backup): 9. replica handle backup clear context and invalid status
Posted by GitBox <gi...@apache.org>.
hycdong commented on code in PR #1150:
URL: https://github.com/apache/incubator-pegasus/pull/1150#discussion_r976320618
##########
src/rdsn/src/replica/backup/test/replica_backup_manager_test.cpp:
##########
@@ -120,7 +143,178 @@ class replica_backup_manager_test : public replica_test_base
std::unique_ptr<replica_backup_manager> _backup_mgr;
};
-// TODO(heyuchen): add unit test for on_backup after implement all status
+TEST_F(replica_backup_manager_test, on_backup_test)
+{
+ fail::cfg("replica_backup_start_checkpointing", "return()");
+ fail::cfg("replica_backup_start_uploading", "return()");
+ fail::cfg("replica_backup_upload_completed", "return()");
+ fail::cfg("replica_backup_clear_context", "return()");
+
+ struct test_struct
+ {
+ backup_status::type local_status;
+ backup_status::type request_status;
+ error_code expected_err;
+ backup_status::type expected_status;
+ } tests[]{
+ // request_status = UNINITIALIZED
+ {backup_status::UNINITIALIZED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::SUCCEED},
+ {backup_status::FAILED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = CHECKPOINTING
+ {backup_status::UNINITIALIZED,
+ backup_status::CHECKPOINTING,
+ ERR_OK,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTING,
+ backup_status::CHECKPOINTING,
+ ERR_OK,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::CHECKPOINTING,
+ ERR_OK,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::SUCCEED},
+ {backup_status::FAILED,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = CHECKPOINTED
+ {backup_status::UNINITIALIZED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::SUCCEED},
+ {backup_status::FAILED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = UPLOADING
+ {backup_status::UNINITIALIZED,
+ backup_status::UPLOADING,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::UPLOADING,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED, backup_status::UPLOADING, ERR_OK, backup_status::UPLOADING},
+ {backup_status::UPLOADING, backup_status::UPLOADING, ERR_OK, backup_status::UPLOADING},
+ {backup_status::SUCCEED, backup_status::UPLOADING, ERR_OK, backup_status::SUCCEED},
+ {backup_status::FAILED, backup_status::UPLOADING, ERR_INVALID_STATE, backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::UPLOADING,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = SUCCEED
+ {backup_status::UNINITIALIZED,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED, backup_status::SUCCEED, ERR_INVALID_STATE, backup_status::SUCCEED},
+ {backup_status::FAILED, backup_status::SUCCEED, ERR_INVALID_STATE, backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = FAILED
+ {backup_status::UNINITIALIZED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::UPLOADING, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::SUCCEED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::FAILED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CANCELED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ // request_status = CANCELED
+ {backup_status::UNINITIALIZED,
+ backup_status::CANCELED,
+ ERR_OK,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::CANCELED,
+ ERR_OK,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTED,
+ backup_status::CANCELED,
+ ERR_OK,
+ backup_status::UNINITIALIZED},
+ {backup_status::UPLOADING, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::SUCCEED, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::FAILED, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CANCELED, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED}};
+
+ for (const auto &test : tests) {
+ ASSERT_EQ(on_backup(test.local_status, test.request_status), test.expected_err);
Review Comment:
To be honest, if some case failed, I also can not figure out which one is failed according to the ci log as you shows, but I can figure out it by unit test log with info logs, usually in my local environment.
Actually, almost all tests including unit tests and function testss use this implementation, improving them is better.
How about use another pr to update all backup tests in a more clear way?
##########
src/rdsn/src/replica/backup/test/replica_backup_manager_test.cpp:
##########
@@ -120,7 +143,178 @@ class replica_backup_manager_test : public replica_test_base
std::unique_ptr<replica_backup_manager> _backup_mgr;
};
-// TODO(heyuchen): add unit test for on_backup after implement all status
+TEST_F(replica_backup_manager_test, on_backup_test)
+{
+ fail::cfg("replica_backup_start_checkpointing", "return()");
+ fail::cfg("replica_backup_start_uploading", "return()");
+ fail::cfg("replica_backup_upload_completed", "return()");
+ fail::cfg("replica_backup_clear_context", "return()");
+
+ struct test_struct
+ {
+ backup_status::type local_status;
+ backup_status::type request_status;
+ error_code expected_err;
+ backup_status::type expected_status;
+ } tests[]{
+ // request_status = UNINITIALIZED
+ {backup_status::UNINITIALIZED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::SUCCEED},
+ {backup_status::FAILED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = CHECKPOINTING
+ {backup_status::UNINITIALIZED,
+ backup_status::CHECKPOINTING,
+ ERR_OK,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTING,
+ backup_status::CHECKPOINTING,
+ ERR_OK,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::CHECKPOINTING,
+ ERR_OK,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::SUCCEED},
+ {backup_status::FAILED,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = CHECKPOINTED
+ {backup_status::UNINITIALIZED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::SUCCEED},
+ {backup_status::FAILED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = UPLOADING
+ {backup_status::UNINITIALIZED,
+ backup_status::UPLOADING,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::UPLOADING,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED, backup_status::UPLOADING, ERR_OK, backup_status::UPLOADING},
+ {backup_status::UPLOADING, backup_status::UPLOADING, ERR_OK, backup_status::UPLOADING},
+ {backup_status::SUCCEED, backup_status::UPLOADING, ERR_OK, backup_status::SUCCEED},
+ {backup_status::FAILED, backup_status::UPLOADING, ERR_INVALID_STATE, backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::UPLOADING,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = SUCCEED
+ {backup_status::UNINITIALIZED,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED, backup_status::SUCCEED, ERR_INVALID_STATE, backup_status::SUCCEED},
+ {backup_status::FAILED, backup_status::SUCCEED, ERR_INVALID_STATE, backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = FAILED
+ {backup_status::UNINITIALIZED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::UPLOADING, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::SUCCEED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::FAILED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CANCELED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ // request_status = CANCELED
+ {backup_status::UNINITIALIZED,
+ backup_status::CANCELED,
+ ERR_OK,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::CANCELED,
+ ERR_OK,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTED,
+ backup_status::CANCELED,
+ ERR_OK,
+ backup_status::UNINITIALIZED},
+ {backup_status::UPLOADING, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::SUCCEED, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::FAILED, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CANCELED, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED}};
+
+ for (const auto &test : tests) {
+ ASSERT_EQ(on_backup(test.local_status, test.request_status), test.expected_err);
Review Comment:
To be honest, if some case failed, I also can not figure out which one is failed according to the ci log as you shows, but I can figure out it by unit test log with info logs, usually in my local environment.
Actually, almost all tests including unit tests and function testss use this implementation, improving them is better.
How about use another pr to update all backup tests in the more clear way?
--
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
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1150: feat(backup): 9. replica handle backup clear context and invalid status
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1150:
URL: https://github.com/apache/incubator-pegasus/pull/1150#discussion_r974855703
##########
src/rdsn/src/common/backup_restore_common.h:
##########
@@ -29,6 +29,7 @@ namespace dsn {
namespace replication {
DSN_DECLARE_string(cold_backup_root);
+DSN_DECLARE_uint32(cold_backup_checkpoint_reserve_minutes);
Review Comment:
It's no needed to declare it in header files, declare it in the cpp files where you use it is OK?
##########
src/rdsn/src/replica/backup/test/replica_backup_manager_test.cpp:
##########
@@ -120,7 +143,178 @@ class replica_backup_manager_test : public replica_test_base
std::unique_ptr<replica_backup_manager> _backup_mgr;
};
-// TODO(heyuchen): add unit test for on_backup after implement all status
+TEST_F(replica_backup_manager_test, on_backup_test)
+{
+ fail::cfg("replica_backup_start_checkpointing", "return()");
+ fail::cfg("replica_backup_start_uploading", "return()");
+ fail::cfg("replica_backup_upload_completed", "return()");
+ fail::cfg("replica_backup_clear_context", "return()");
+
+ struct test_struct
+ {
+ backup_status::type local_status;
+ backup_status::type request_status;
+ error_code expected_err;
+ backup_status::type expected_status;
+ } tests[]{
+ // request_status = UNINITIALIZED
+ {backup_status::UNINITIALIZED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::SUCCEED},
+ {backup_status::FAILED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = CHECKPOINTING
+ {backup_status::UNINITIALIZED,
+ backup_status::CHECKPOINTING,
+ ERR_OK,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTING,
+ backup_status::CHECKPOINTING,
+ ERR_OK,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::CHECKPOINTING,
+ ERR_OK,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::SUCCEED},
+ {backup_status::FAILED,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = CHECKPOINTED
+ {backup_status::UNINITIALIZED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::SUCCEED},
+ {backup_status::FAILED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = UPLOADING
+ {backup_status::UNINITIALIZED,
+ backup_status::UPLOADING,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::UPLOADING,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED, backup_status::UPLOADING, ERR_OK, backup_status::UPLOADING},
+ {backup_status::UPLOADING, backup_status::UPLOADING, ERR_OK, backup_status::UPLOADING},
+ {backup_status::SUCCEED, backup_status::UPLOADING, ERR_OK, backup_status::SUCCEED},
+ {backup_status::FAILED, backup_status::UPLOADING, ERR_INVALID_STATE, backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::UPLOADING,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = SUCCEED
+ {backup_status::UNINITIALIZED,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED, backup_status::SUCCEED, ERR_INVALID_STATE, backup_status::SUCCEED},
+ {backup_status::FAILED, backup_status::SUCCEED, ERR_INVALID_STATE, backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = FAILED
+ {backup_status::UNINITIALIZED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::UPLOADING, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::SUCCEED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::FAILED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CANCELED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ // request_status = CANCELED
+ {backup_status::UNINITIALIZED,
+ backup_status::CANCELED,
+ ERR_OK,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::CANCELED,
+ ERR_OK,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTED,
+ backup_status::CANCELED,
+ ERR_OK,
+ backup_status::UNINITIALIZED},
+ {backup_status::UPLOADING, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::SUCCEED, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::FAILED, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CANCELED, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED}};
+
+ for (const auto &test : tests) {
+ ASSERT_EQ(on_backup(test.local_status, test.request_status), test.expected_err);
Review Comment:
if any test case failed, it would be a bit of harder to find out which one failed? add some message after ASSERT_* may help, for example the index of `tests`.
e.g.
```
int i = 0;
...
ASSERT_EQ(...) << i++;
```
##########
src/rdsn/src/replica/backup/replica_backup_manager.cpp:
##########
@@ -414,7 +422,70 @@ void replica_backup_manager::clear_context()
{
FAIL_POINT_INJECT_F("replica_backup_clear_context",
[this](dsn::string_view) { _status = backup_status::UNINITIALIZED; });
- // TODO(heyuchen): TBD
+
+ zauto_write_lock l(_lock);
+ if (_status == backup_status::UNINITIALIZED) {
+ return;
+ }
+ ddebug_replica("start to clear backup context, old status = {}", enum_to_string(_status));
+
+ for (auto &kv : _upload_files_task) {
Review Comment:
```suggestion
for (const auto &kv : _upload_files_task) {
```
--
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
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1150: feat(backup): 9. replica handle backup clear context and invalid status
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1150:
URL: https://github.com/apache/incubator-pegasus/pull/1150#discussion_r976227700
##########
src/rdsn/src/replica/backup/test/replica_backup_manager_test.cpp:
##########
@@ -120,7 +143,178 @@ class replica_backup_manager_test : public replica_test_base
std::unique_ptr<replica_backup_manager> _backup_mgr;
};
-// TODO(heyuchen): add unit test for on_backup after implement all status
+TEST_F(replica_backup_manager_test, on_backup_test)
+{
+ fail::cfg("replica_backup_start_checkpointing", "return()");
+ fail::cfg("replica_backup_start_uploading", "return()");
+ fail::cfg("replica_backup_upload_completed", "return()");
+ fail::cfg("replica_backup_clear_context", "return()");
+
+ struct test_struct
+ {
+ backup_status::type local_status;
+ backup_status::type request_status;
+ error_code expected_err;
+ backup_status::type expected_status;
+ } tests[]{
+ // request_status = UNINITIALIZED
+ {backup_status::UNINITIALIZED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::SUCCEED},
+ {backup_status::FAILED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::UNINITIALIZED,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = CHECKPOINTING
+ {backup_status::UNINITIALIZED,
+ backup_status::CHECKPOINTING,
+ ERR_OK,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTING,
+ backup_status::CHECKPOINTING,
+ ERR_OK,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::CHECKPOINTING,
+ ERR_OK,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::SUCCEED},
+ {backup_status::FAILED,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::CHECKPOINTING,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = CHECKPOINTED
+ {backup_status::UNINITIALIZED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::SUCCEED},
+ {backup_status::FAILED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::CHECKPOINTED,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = UPLOADING
+ {backup_status::UNINITIALIZED,
+ backup_status::UPLOADING,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::UPLOADING,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED, backup_status::UPLOADING, ERR_OK, backup_status::UPLOADING},
+ {backup_status::UPLOADING, backup_status::UPLOADING, ERR_OK, backup_status::UPLOADING},
+ {backup_status::SUCCEED, backup_status::UPLOADING, ERR_OK, backup_status::SUCCEED},
+ {backup_status::FAILED, backup_status::UPLOADING, ERR_INVALID_STATE, backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::UPLOADING,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = SUCCEED
+ {backup_status::UNINITIALIZED,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTING},
+ {backup_status::CHECKPOINTED,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::CHECKPOINTED},
+ {backup_status::UPLOADING,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::UPLOADING},
+ {backup_status::SUCCEED, backup_status::SUCCEED, ERR_INVALID_STATE, backup_status::SUCCEED},
+ {backup_status::FAILED, backup_status::SUCCEED, ERR_INVALID_STATE, backup_status::FAILED},
+ {backup_status::CANCELED,
+ backup_status::SUCCEED,
+ ERR_INVALID_STATE,
+ backup_status::CANCELED},
+ // request_status = FAILED
+ {backup_status::UNINITIALIZED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::UPLOADING, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::SUCCEED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::FAILED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CANCELED, backup_status::FAILED, ERR_OK, backup_status::UNINITIALIZED},
+ // request_status = CANCELED
+ {backup_status::UNINITIALIZED,
+ backup_status::CANCELED,
+ ERR_OK,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTING,
+ backup_status::CANCELED,
+ ERR_OK,
+ backup_status::UNINITIALIZED},
+ {backup_status::CHECKPOINTED,
+ backup_status::CANCELED,
+ ERR_OK,
+ backup_status::UNINITIALIZED},
+ {backup_status::UPLOADING, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::SUCCEED, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::FAILED, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED},
+ {backup_status::CANCELED, backup_status::CANCELED, ERR_OK, backup_status::UNINITIALIZED}};
+
+ for (const auto &test : tests) {
+ ASSERT_EQ(on_backup(test.local_status, test.request_status), test.expected_err);
Review Comment:
As the test log shown [1], I think common developer except some one, like you, who is familar with it, are not able to distinguish which test case fail (if any one failed).
:)
It's just a suggestion, it's not a block for this PR.
1. https://github.com/apache/incubator-pegasus/actions/runs/3094760729/jobs/5010167970#step:8:9471
--
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
[GitHub] [incubator-pegasus] hycdong commented on a diff in pull request #1150: feat(backup): 9. replica handle backup clear context and invalid status
Posted by GitBox <gi...@apache.org>.
hycdong commented on code in PR #1150:
URL: https://github.com/apache/incubator-pegasus/pull/1150#discussion_r976082780
##########
src/rdsn/src/common/backup_restore_common.h:
##########
@@ -29,6 +29,7 @@ namespace dsn {
namespace replication {
DSN_DECLARE_string(cold_backup_root);
+DSN_DECLARE_uint32(cold_backup_checkpoint_reserve_minutes);
Review Comment:
Maybe other configuration can declare in cpp file, but as my comment this config `cold_backup_checkpoint_reserve_minutes` will be used in different cpp files, it need to be defined in header file.
--
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
[GitHub] [incubator-pegasus] hycdong commented on a diff in pull request #1150: feat(backup): 9. replica handle backup clear context and invalid status
Posted by GitBox <gi...@apache.org>.
hycdong commented on code in PR #1150:
URL: https://github.com/apache/incubator-pegasus/pull/1150#discussion_r976082780
##########
src/rdsn/src/common/backup_restore_common.h:
##########
@@ -29,6 +29,7 @@ namespace dsn {
namespace replication {
DSN_DECLARE_string(cold_backup_root);
+DSN_DECLARE_uint32(cold_backup_checkpoint_reserve_minutes);
Review Comment:
Maybe other configuration can declare in cpp file, but as my comment shows this config `cold_backup_checkpoint_reserve_minutes` will be used in different cpp files, it need to be defined in header file.
--
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
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1150: feat(backup): 9. replica handle backup clear context and invalid status
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1150:
URL: https://github.com/apache/incubator-pegasus/pull/1150#discussion_r976022967
##########
docker/README.md:
##########
@@ -27,7 +27,7 @@ to deploying a standalone cluster of Pegasus containers on your local machine.
[](https://github.com/apache/incubator-pegasus/actions/workflows/thirdparty-regular-push.yml)
-[](https://github.com/apache/incubator-pegasus/actions/workflows/pegasus-regular-build.yml)
+[](https://github.com/apache/incubator-pegasus/actions/workflows/regular-build.yml)
Review Comment:
Rebase the master branch could solve this problem, it has been fixed.
##########
src/rdsn/src/common/backup_restore_common.h:
##########
@@ -29,6 +29,7 @@ namespace dsn {
namespace replication {
DSN_DECLARE_string(cold_backup_root);
+DSN_DECLARE_uint32(cold_backup_checkpoint_reserve_minutes);
Review Comment:
Larger header file will cause build slower
--
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
[GitHub] [incubator-pegasus] hycdong commented on a diff in pull request #1150: feat(backup): 9. replica handle backup clear context and invalid status
Posted by GitBox <gi...@apache.org>.
hycdong commented on code in PR #1150:
URL: https://github.com/apache/incubator-pegasus/pull/1150#discussion_r976095026
##########
src/rdsn/src/common/backup_restore_common.h:
##########
@@ -29,6 +29,7 @@ namespace dsn {
namespace replication {
DSN_DECLARE_string(cold_backup_root);
+DSN_DECLARE_uint32(cold_backup_checkpoint_reserve_minutes);
Review Comment:
In my memory, it will be used in at least 3 files in future pr. Will declaring it in different 3 cpp files is more effecitive than declaring it in one header file? I just don't know which way is more effective.
--
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
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1150: feat(backup): 9. replica handle backup clear context and invalid status
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1150:
URL: https://github.com/apache/incubator-pegasus/pull/1150#discussion_r976213538
##########
src/rdsn/src/common/backup_restore_common.h:
##########
@@ -29,6 +29,7 @@ namespace dsn {
namespace replication {
DSN_DECLARE_string(cold_backup_root);
+DSN_DECLARE_uint32(cold_backup_checkpoint_reserve_minutes);
Review Comment:
I think it's a type of combine rule of Include_What_You_Use and Forward_Declarations. If the header file dose use the header/type itself, avoid including / declearing it in header files, include or declear it in the place where really use it.
1. https://google.github.io/styleguide/cppguide.html#Include_What_You_Use
2. https://google.github.io/styleguide/cppguide.html#Forward_Declarations
--
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