hycdong commented on code in PR #1112:
URL: https://github.com/apache/incubator-pegasus/pull/1112#discussion_r948580634


##########
src/rdsn/src/common/backup.thrift:
##########
@@ -64,24 +64,28 @@ struct configuration_restore_request
     9:optional string   restore_path;
 }
 
+// meta -> replica
 struct backup_request
 {
-    1:dsn.gpid              pid;
-    2:policy_info           policy;
-    3:string                app_name;
-    4:i64                   backup_id;
+    1:dsn.gpid          pid;
+    2:string            app_name;
+    3:i64               backup_id;
+    4:backup_status     status;
+    5:string            backup_provider_type;
     // user specified backup_path.
-    5:optional string       backup_path;
+    6:optional string   backup_root_path;
 }
 
 struct backup_response
 {
     1:dsn.error_code    err;
     2:dsn.gpid          pid;
-    3:i32               progress;  // the progress of the cold_backup
-    4:string            policy_name;
-    5:i64               backup_id;
-    6:i64               checkpoint_total_size;
+    3:i64               backup_id;
+    4:backup_status     status;
+    5:optional dsn.error_code   checkpoint_err;
+    6:optional dsn.error_code   upload_err;

Review Comment:
   Actually, bulk load still seperete rpc_error, download_error and 
ingestion_err, you can reference sturcture `partition_bulk_load_state`.
   
   And in my current design, won't have the too many if condition in your 
example. In my design, it just like
   ```
   if (rpc != ok) { // handle this condition }
   if (resp.err != ok) { // handle this condition }
   if (resp.checkpoint_upload_err != ok) { // handle this condition }
   ```
   If I combine error and checkpoint_upload_err into one, the code will be like:
   ```
   if (rpc != ok) { // handle this condition }
   if (resp.err != ok) { 
       if (original resp.err != ok){
           // handle this condition
       }
       if (original resp.checkpoint_upload_err != ok ) {
           // handle this condition
       }
   }
   ```
   
   I think it is okay to have two errors to distinguish different errors, if 
there is only one field redundant, but can make structure clear. In my previous 
design, checkpoint error and upload error also should be seperated, I have 
already compromised my logic :-)



-- 
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]

Reply via email to