Support fe heartbeat use thrift protocol to get stable response #5027
Conversation
| } | ||
|
|
||
| struct TFrontendPingFrontendRequest { | ||
| 1: required i32 clusterId |
There was a problem hiding this comment.
required field should be used very carefully, reference here
There was a problem hiding this comment.
I think if the filed is really required and the TFrontendPingFrontendRequest is firstly constructed to use, the field used requried is ok
gensrc/thrift/FrontendService.thrift
Outdated
| @@ -689,4 +708,6 @@ service FrontendService { | |||
| TStreamLoadPutResult streamLoadPut(1: TStreamLoadPutRequest request) | |||
|
|
|||
| Status.TStatus snapshotLoaderReport(1: TSnapshotLoaderReportRequest request) | |||
| } | |||
|
|
|||
| TBootstrapResult ping(1: TFrontendPingFrontendRequest request) | |||
There was a problem hiding this comment.
The request and response names are unmatched with the service name.
There was a problem hiding this comment.
thank you for your advice, I will change it.
There was a problem hiding this comment.
If service is named as ping, why not just name the request/response as TPingRequest/TPingResponse. I'm just curious about the naming rule of Doris, it's kind of weird for me.
There was a problem hiding this comment.
Sorry to intervene, It's just my personal options, you can just ignore them all ^_^
There was a problem hiding this comment.
In my option, by TFrontendPingFrontendRequest, we can know the request is sent by fe to fe, by TPingRequest is less information?
There was a problem hiding this comment.
I get what you mean, actually, TFrontendPingFrontendRequest is like the name in
TBrokerOperationStatus ping(1: TBrokerPingBrokerRequest request);
| } | ||
|
|
||
| struct TBootstrapResult { | ||
| 1: required TFrontendPingFrontendStatusCode status |
There was a problem hiding this comment.
Is it ok to use TStatus here?
There was a problem hiding this comment.
TStatus is used for be, just like TBrokerOperationStatus is used for broker, so I think use new status for fe would be better
There was a problem hiding this comment.
Didn't find any specification that TStatus can only be used between BE and FE.
https://github.com/apache/incubator-doris/blob/49f7eb69bfa9190612e84d5cbe989fe15fb4e995/gensrc/thrift/Status.thrift#L22
There was a problem hiding this comment.
Yes, I am also not sure to reuse the TStatus or just like
enum TBrokerOperationStatusCode {
OK = 0;
END_OF_FILE = 301;
// user input error
NOT_AUTHORIZED = 401;
DUPLICATE_REQUEST = 402;
INVALID_INPUT_OFFSET = 403; // user input offset is invalid, is large than file length
INVALID_INPUT_FILE_PATH = 404;
INVALID_ARGUMENT = 405;
// internal server error
FILE_NOT_FOUND = 501;
TARGET_STORAGE_SERVICE_ERROR = 502; // the target storage service error
OPERATION_NOT_SUPPORTED = 503; // the api is not implemented
}
define new status struct
| if (Config.enable_fe_heartbeat_by_thrift) { | ||
| return getHeartbeatResponseByThrift(); | ||
| } else { | ||
| return getHeartbeatResponseByHttp(); |
There was a problem hiding this comment.
Is it really necessary to keep the HTTP based codes?
cc @kangkaisen
There was a problem hiding this comment.
@sduzh be compatible with older versions(0.13), and should be moved next version(0.15). if you ugrade non master fe first, the heartbeat will fail.
…esponse (apache#5027) This PR is to support fe master get fe heartbeat response by thrift protocol instead of http protocol.
Proposed changes
This PR is to support fe master get fe heartbeat response by thrift protocol instead of http protocol.
Types of changes
What types of changes does your code introduce to Doris?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...