feat(backup): 4. meta server send backup_request to replica

[hycdong]

#1081

This pull request implement that meta server send backup_request to replica server:

1. meta server create backup_engine instance by init_backup, already in #1102
2. write app_metadata file into remote file system by calling write_app_info
3. update app backup_status from UNINITIALIZED into CHECKPOINTING by calling update_backup_item_on_remote_storage
4. send backup_request to all primary partition by calling backup_app_partition
5. handle response and resend request by calling on_backup_reply, will be implemented in further pr

Besides, this pr also update following related content:

• update backup_request and backup_response in thrift
• implement some common functions in backup_restore_common
• rename cold_backup_constant into backup_constant
• update cold_backup_root config into gflags
• add related unit test

[foreverneverer]

why need two error_code? using one-error_code + hint is not ok?

[hycdong]

Good idea, meta server can distinguish error with its backup_status.
Done.

[foreverneverer]

Ok, what the different between 1:dsn.error_code err and 5:optional dsn.error_code checkpoint_upload_err;?

[hycdong]

err is used for backup_request rpc, such as ERR_OK, ERR_INVALID_STATE. checkpoint_upload_err is used for backup checkpoint and upload. If backup checkpoint failed, checkpoint_upload_err won't be error_ok.

[foreverneverer]

So I think that just err is still OK. In other rpc, I remember that we have always defined it using one err.

[foreverneverer]

With your design, the sender needs to judge the response as follows:

if (rpc == ok) {
     if ( resp.err == ok) {
         if (resp.checkpoint_err == ok) {
             /** if you define `more clear err code as you say`, maybe need:**/
              /** if (err_a == ...) {
                        if (err_b == ...)
}
}
}
}

This logic is also redundant. don't suggest this design, you can refer the origin rpc defination, I think the origin is elegant

[hycdong]

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 :-)

[foreverneverer]

Well, wait other's opinion, if they have no objections, I will be willing think it is a reasonable design

[hycdong]

@acelyc111 Please give us your suggestion about this comment~ @foreverneverer has different opion with me, and we can not persude each other.

[acelyc111]

If I didn't miss anything, I didn't find where checkpoint_upload_err is used currently. We keep this point in mind and see how will it be used later, if find it's not so elegant, we can point it out then.

[acelyc111]

Will you use another RPC code? Is there any problem if a user use old version shell-tool attempt to control the cluster?

[hycdong]

This rpc is not used from shell-tool to meta server, is meta server to replica server, won't trigger the control problem.

[acelyc111]

OK, thanks.
I meant how to keep compatablity, if the meta servers are in new version, and the replica server are in old version, what will happen if we ask the cluster to do backup? Is it neccessary to add new rpc code for the new implemention?

[hycdong]

If meta server is in new version, replica server in old version. I don't consider this condition compatible, I think it is a dangerous case for meta server and replica server has different version, because new version especially a feature version, meta server will provide mamy new rpc which replica server can not recognize, and it is not necessary to add new rpc code for compatible only when new meta and old replica case.

[acelyc111]

Rolling update is a common case which many user have to face. We will not ensure every feature should work well at this case, but at least avoid the server crash.
If we rewrite the RPC message, the related RPC code would better to add a new one, and left the old one as deprecated.

[hycdong]

Rolling update is a common case, but it is not recommended to update meta firstly, we recommendly update replica server firstly, so it is a seldom case that meta is new version but replica is old version.

[acelyc111]

No matter which one is new version. The new replica server will still crash if receive a old backup request from old version meta server?

[acelyc111]

LGTM
But still doubt if there is any compatablity problem.

[hycdong]

LGTM But still doubt if there is any compatablity problem.

Okay, I will add compatiable explaination after all code merged.