Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement request subcommands #157

Merged
merged 5 commits into from
Jan 8, 2025
Merged

implement request subcommands #157

merged 5 commits into from
Jan 8, 2025

Conversation

t-horikawa
Copy link
Contributor

@t-horikawa t-horikawa requested a review from kuron99 January 6, 2025 00:54
@t-horikawa
Copy link
Contributor Author

@kuron99 下記についてreviewをお願いします。
なお、エラー(REQUEST_FAILURE_EXCEPTION, STATEMENT_NOT_FOUND_EXCEPTION)はerror.protoに追加されるのだと思っていますが、その修正は入っていません(ので、修正案をお願いします)。

// Requests to extract executing statement info in payload data.
message ExtractStatementInfo {
// session ID
uint64 session_id = 1;
// payload data as plain byte array.
bytes payload = 2;
}

ExtractStatementInfo extract_statement_info = 22;

// Response of ExtractStatementInfo.
message ExtractStatementInfo {
reserved 1 to 10;
// the response body.
oneof result {
// request is successfully completed.
Success success = 11;
// error was occurred.
Error error = 12;
}
// request is successfully completed.
message Success {
// the transaction ID for the statement.
oneof transaction_id_opt {
// the corresponding transaction ID.
jogasaki.proto.sql.common.TransactionId transaction_id = 1;
}
// the executing SQL text of the statement.
oneof sql_opt {
// SQL statement string.
string sql = 2;
}
}
}

constexpr static std::size_t SQL_MESSAGE_VERSION_MAJOR = 1;
constexpr static std::size_t SQL_MESSAGE_VERSION_MINOR = 4;

response.protoですが、他のコマンドと同じようにoneof responseの一つとして返す方が良いのでしたら、その修正案もお願いします。

@kuron99
Copy link
Contributor

kuron99 commented Jan 7, 2025

少し修正してPRのブランチにpushしました。下記は変更内容とその理由です。これで問題なければmasterにマージお願いします。

  • 他のメッセージと形式を合わせるために、response::ExtractStatementInfo 内のSuccessメッセージの位置を移動させました
  • REQUEST_FAILURE_EXCEPTION, STATEMENT_NOT_FOUND_EXCEPTION はすでにerror.protoにあり、それが使えると思います。
    (参照: https://github.com/project-tsurugi/jogasaki/blob/9eada2dac1373933f1ebb1513b7aabb496b27894/src/jogasaki/proto/sql/error.proto#L111 )
  • 互換性の観点から、ExtractStatementInfoは他のコマンドと同じようにoneof responseの一つとして返す方がいいと思います。トップレベルのメッセージにしてしまうと古いバージョンの処理系からは完全に謎のメッセージになってしまい、deserializeが失敗する可能性があるのですが、response::Responseのoneofの一つとして返す分には読み飛ばすことが可能なので少しcompatibilityがましになります。
  • 上の変更であれば既存のメッセージへの追加なのでSMV minor updateでいいはずです

@t-horikawa t-horikawa merged commit b8e113d into master Jan 8, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants