This is an automated email from the ASF dual-hosted git repository. kxiao pushed a commit to branch branch-2.0 in repository https://gitbox.apache.org/repos/asf/doris.git
commit dbfed0056fcba35893bf7cc99631356f5cd7f69c Author: Jack Drogon <[email protected]> AuthorDate: Thu Jul 20 10:43:05 2023 +0800 [Enhancement](http) Add HttpError to HttpClient::execute_with_retry (#21989) --- be/src/common/status.h | 2 ++ be/src/http/action/download_binlog_action.cpp | 33 +++++++++++++++++---------- be/src/http/http_client.cpp | 10 +++++++- gensrc/thrift/Status.thrift | 3 +++ 4 files changed, 35 insertions(+), 13 deletions(-) diff --git a/be/src/common/status.h b/be/src/common/status.h index 1ccf5bec5f..b2d5cf7d67 100644 --- a/be/src/common/status.h +++ b/be/src/common/status.h @@ -57,6 +57,7 @@ TStatusError(ABORTED); TStatusError(DATA_QUALITY_ERROR); TStatusError(LABEL_ALREADY_EXISTS); TStatusError(NOT_AUTHORIZED); +TStatusError(HTTP_ERROR); #undef TStatusError // BE internal errors E(OS_ERROR, -100); @@ -400,6 +401,7 @@ public: ERROR_CTOR(Aborted, ABORTED) ERROR_CTOR(DataQualityError, DATA_QUALITY_ERROR) ERROR_CTOR(NotAuthorized, NOT_AUTHORIZED) + ERROR_CTOR(HttpError, HTTP_ERROR) #undef ERROR_CTOR template <int code> diff --git a/be/src/http/action/download_binlog_action.cpp b/be/src/http/action/download_binlog_action.cpp index 1e2ea0e36f..7548328a83 100644 --- a/be/src/http/action/download_binlog_action.cpp +++ b/be/src/http/action/download_binlog_action.cpp @@ -77,10 +77,19 @@ void handle_get_binlog_info(HttpRequest* req) { auto tablet = get_tablet(tablet_id); const auto& [rowset_id, num_segments] = tablet->get_binlog_info(binlog_version); - auto binlog_info_msg = fmt::format("{}:{}", rowset_id, num_segments); - HttpChannel::send_reply(req, binlog_info_msg); + if (rowset_id.empty()) { + HttpChannel::send_reply( + req, HttpStatus::NOT_FOUND, + fmt::format("get binlog info failed, binlog_version={}", binlog_version)); + } else if (num_segments < 0) { + HttpChannel::send_reply(req, HttpStatus::INTERNAL_SERVER_ERROR, + fmt::format("invalid num_segments: {}", num_segments)); + } else { + auto binlog_info_msg = fmt::format("{}:{}", rowset_id, num_segments); + HttpChannel::send_reply(req, binlog_info_msg); + } } catch (const std::exception& e) { - HttpChannel::send_reply(req, e.what()); + HttpChannel::send_reply(req, HttpStatus::INTERNAL_SERVER_ERROR, e.what()); LOG(WARNING) << "get binlog info failed, error: " << e.what(); return; } @@ -97,7 +106,7 @@ void handle_get_segment_file(HttpRequest* req) { const auto& segment_index = get_http_param(req, kSegmentIndexParameter); segment_file_path = tablet->get_segment_filepath(rowset_id, segment_index); } catch (const std::exception& e) { - HttpChannel::send_reply(req, e.what()); + HttpChannel::send_reply(req, HttpStatus::INTERNAL_SERVER_ERROR, e.what()); LOG(WARNING) << "get download file path failed, error: " << e.what(); return; } @@ -107,12 +116,12 @@ void handle_get_segment_file(HttpRequest* req) { bool exists = false; Status status = io::global_local_filesystem()->exists(segment_file_path, &exists); if (!status.ok()) { - HttpChannel::send_reply(req, status.to_string()); + HttpChannel::send_reply(req, HttpStatus::INTERNAL_SERVER_ERROR, status.to_string()); LOG(WARNING) << "check file exists failed, error: " << status.to_string(); return; } if (!exists) { - HttpChannel::send_reply(req, "file not exist."); + HttpChannel::send_reply(req, HttpStatus::NOT_FOUND, "file not exist."); LOG(WARNING) << "file not exist, file path: " << segment_file_path; return; } @@ -127,14 +136,13 @@ void handle_get_rowset_meta(HttpRequest* req) { const auto& binlog_version = get_http_param(req, kBinlogVersionParameter); auto rowset_meta = tablet->get_binlog_rowset_meta(binlog_version, rowset_id); if (rowset_meta.empty()) { - // TODO(Drogon): send error - HttpChannel::send_reply(req, + HttpChannel::send_reply(req, HttpStatus::NOT_FOUND, fmt::format("get rowset meta failed, rowset_id={}", rowset_id)); } else { HttpChannel::send_reply(req, rowset_meta); } } catch (const std::exception& e) { - HttpChannel::send_reply(req, e.what()); + HttpChannel::send_reply(req, HttpStatus::INTERNAL_SERVER_ERROR, e.what()); LOG(WARNING) << "get download file path failed, error: " << e.what(); } } @@ -147,7 +155,8 @@ void DownloadBinlogAction::handle(HttpRequest* req) { VLOG_CRITICAL << "accept one download binlog request " << req->debug_string(); if (!config::enable_feature_binlog) { - HttpChannel::send_reply(req, "binlog feature is not enabled."); + HttpChannel::send_reply(req, HttpStatus::INTERNAL_SERVER_ERROR, + "binlog feature is not enabled."); return; } @@ -157,7 +166,7 @@ void DownloadBinlogAction::handle(HttpRequest* req) { // FIXME(Drogon): support check token // status = _check_token(req); if (!status.ok()) { - HttpChannel::send_reply(req, status.to_string()); + HttpChannel::send_reply(req, HttpStatus::UNAUTHORIZED, status.to_string()); return; } } @@ -175,7 +184,7 @@ void DownloadBinlogAction::handle(HttpRequest* req) { } else { auto error_msg = fmt::format("invalid method: {}", method); LOG(WARNING) << error_msg; - HttpChannel::send_reply(req, error_msg); + HttpChannel::send_reply(req, HttpStatus::NOT_IMPLEMENTED, error_msg); } } diff --git a/be/src/http/http_client.cpp b/be/src/http/http_client.cpp index 0919d21d38..9c3298632c 100644 --- a/be/src/http/http_client.cpp +++ b/be/src/http/http_client.cpp @@ -24,6 +24,7 @@ #include <ostream> #include "common/config.h" +#include "http/http_status.h" #include "util/stack_util.h" namespace doris { @@ -229,7 +230,14 @@ Status HttpClient::execute_with_retry(int retry_times, int sleep_time, HttpClient client; status = callback(&client); if (status.ok()) { - return status; + auto http_status = static_cast<HttpStatus>(client.get_http_status()); + if (http_status == 200) { + return status; + } else { + auto error_msg = fmt::format("http status code is not 200, status={}", http_status); + LOG(WARNING) << error_msg; + return Status::HttpError(error_msg); + } } sleep(sleep_time); } diff --git a/gensrc/thrift/Status.thrift b/gensrc/thrift/Status.thrift index 0342f9bce0..0d1931d8ec 100644 --- a/gensrc/thrift/Status.thrift +++ b/gensrc/thrift/Status.thrift @@ -94,6 +94,9 @@ enum TStatusCode { // Snapshot Related from 70 SNAPSHOT_NOT_EXIST = 70, + + // BE Status HTTP_ERROR + HTTP_ERROR = 71, } struct TStatus { --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
