feat: add dbname and health check api #18
Conversation
| @@ -22,6 +22,8 @@ message RequestHeader { | |||
| string schema = 2; | |||
| // The `authorization` header, much like http's authorization header. | |||
| AuthHeader authorization = 3; | |||
| // The `dbname` for the request | |||
| string dbname = 4; | |||
There was a problem hiding this comment.
Since "dbname = catalog + '-' + schema", I suggest using "oneof":
message RequestHeader {
message CatalogAndSchema {
string catalog = 1;
string schema = 2;
}
oneof database_ident {
CatalogAndSchema catalog_and_schema = 1;
string dbname = 2;
}
AuthHeader authorization = 3;
}There was a problem hiding this comment.
This is preferred solution, to have consistency in proto semantic and implementation. However, I'm afraid this will become backward incompatible so I chose to append dbname to the header.
There was a problem hiding this comment.
Well, currently we have only one outside use of grpc client, and the one is in early adoption stage. I think we can afford to make this breaking change?
There was a problem hiding this comment.
Why don't we consider keeping the proto unchanged and just splitting/combining the catalog-schema at the upper level?
There was a problem hiding this comment.
Since the proto is also used in our intra-cluster communication, changing this can result in a incompatible upgrade from existing cluster..
There was a problem hiding this comment.
Do we really need the dbname in proto file? Can it be handled at the code level?
There was a problem hiding this comment.
Yes, but here I try to avoid leaking dbname generation pattern to client, for better extensibility. The pattern catalog-schema we are using for now might not be a perfect solution.
I hereby agree to the terms of the GreptimeDB CLA
What's changed and what's your intention?
dbnamefor consistent with protocols like mysql and httpChecklist
Refer to a related PR or issue link (optional)