Skip to content
This repository has been archived by the owner on May 10, 2022. It is now read-only.

refactor(rpc): refactor request meta to adapt to new version of rpc protocol #82

Merged
merged 21 commits into from
Feb 28, 2020

Conversation

levy5307
Copy link
Contributor

@levy5307 levy5307 commented Dec 30, 2019

Refactor our thrift message parser by redesigning the request header format, to support adding is_backup_request new field.

Side effect:
This pull request will import incompatibility.

old/new client --> new server ok.
old client --> old server is ok, too.
but new client --> old server is not ok.
Because the connection will be disconnected by server. So we can`t get any information about version incompatibility, it simply disconnects the connection.

idl/base.thrift Outdated Show resolved Hide resolved
scripts/travis.sh Show resolved Hide resolved
@hycdong hycdong added backup_request backup request feature enhancement New feature or request labels Feb 28, 2020
hycdong
hycdong previously approved these changes Feb 28, 2020
acelyc111
acelyc111 previously approved these changes Feb 28, 2020
@levy5307 levy5307 dismissed stale reviews from acelyc111 and hycdong via 6fb9c85 February 28, 2020 06:09
@@ -31,3 +31,10 @@ struct query_cfg_response
4:bool is_stateful;
5:list<partition_configuration> partitions;
}

struct request_meta {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These fields should be defined optional, be consistent with what server-side defines.

Actually I suggest just copy the thrift codes from pegasus.

// Metadata field of the request in rDSN's thrift protocol (version 1).                                                                                                                                   
// TODO(wutao1): add design doc of the thrift protocol.                                                                                                                                                   
struct thrift_request_meta_v1                                                                                                                                                                             
{                                                                                                                                                                                                         
    // The replica's gpid.                                                                                                                                                                                
    1:optional i32 app_id;                                                                                                                                                                                
    2:optional i32 partition_index;                                                                                                                                                                       
                                                                                                                                                                                                          
    // The timeout of this request that's set on client side.                                                                                                                                             
    3:optional i32 client_timeout;                                                                                                                                                                        
                                                                                                                                                                                                          
    // The hash value calculated from the hash key.                                                                                                                                                       
    4:optional i64 client_partition_hash;                                                                                                                                                                 
                                                                                                                                                                                                          
    // Whether it is a backup request. If true, this request (only if it's a read) can be handled by                                                                                                      
    // a secondary replica, which does not guarantee strong consistency.                                                                                                                                  
    5:optional bool is_backup_request;                                                                                                                                                                    
} 

Copy link
Contributor Author

@levy5307 levy5307 Feb 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don`t suggest to use optional in java client. We use optional in pegasus server because we want it to have good compatibility, so we can delete these data in later version of java client. But in this version, these data is a must.

foreverneverer
foreverneverer previously approved these changes Feb 28, 2020
@foreverneverer foreverneverer dismissed their stale review February 28, 2020 08:57

it‘s misjudgments

@neverchanje neverchanje changed the title refactor(rpc): refactor request meta refactor(rpc): refactor request meta to adapt to new version of rpc protocol Feb 28, 2020
@levy5307 levy5307 merged commit d4a30a0 into XiaoMi:thrift-0.11.0-inlined Feb 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backup_request backup request feature enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants