brbzull0 commented on code in PR #10112:
URL: https://github.com/apache/trafficserver/pull/10112#discussion_r1278978099
##########
mgmt/config/FileManager.cc:
##########
@@ -163,17 +161,20 @@ FileManager::getConfigObj(const char *fileName,
ConfigManager **rbPtr)
return found;
}
-ts::Errata
+swoc::Errata
FileManager::fileChanged(std::string const &fileName, std::string const
&configName)
{
Debug("filemanager", "file changed %s", fileName.c_str());
- ts::Errata ret;
+ swoc::Errata ret;
std::lock_guard<std::mutex> guard(_callbacksMutex);
for (auto const &call : _configCallbacks) {
if (auto const &r = call(fileName, configName); !r) {
Debug("filemanager", "something back from callback %s",
fileName.c_str());
- std::for_each(r.begin(), r.end(), [&ret](auto &&e) { ret.push(e); });
+ if (ret.empty()) {
+ ret.note("Errors while reloading configurations.");
+ }
+ ret.note(r);
Review Comment:
I think we can just do:
```cpp
if (!r.empty()) {
ret.note(r);
}
```
or
```cpp
ret.note(r); // if note() checks ignored r if is empty.
```
The whole idea behing the `for_each` was to add all the traced errors from
one errata to the other, so I think a single `ret.note(other)` should do the
magic.
##########
mgmt/config/FileManager.cc:
##########
@@ -278,13 +285,19 @@ FileManager::rereadConfig()
if (found && enabled) {
if (auto const &r =
fileChanged("proxy.config.body_factory.template_sets_dir",
"proxy.config.body_factory.template_sets_dir");
!r) {
- std::for_each(r.begin(), r.end(), [&ret](auto &&e) { ret.push(e); });
+ if (ret.empty()) {
+ ret.note("Error while loading body factory templates");
+ }
+ ret.note(r);
}
}
if (auto const &r =
fileChanged("proxy.config.ssl.server.ticket_key.filename",
"proxy.config.ssl.server.ticket_key.filename");
!r) {
- std::for_each(r.begin(), r.end(), [&ret](auto &&e) { ret.push(e); });
+ if (ret.empty()) {
+ ret.note("Error while loading ticket keys");
+ }
+ ret.note(r);
Review Comment:
same
##########
mgmt/config/FileManager.cc:
##########
@@ -278,13 +285,19 @@ FileManager::rereadConfig()
if (found && enabled) {
if (auto const &r =
fileChanged("proxy.config.body_factory.template_sets_dir",
"proxy.config.body_factory.template_sets_dir");
!r) {
- std::for_each(r.begin(), r.end(), [&ret](auto &&e) { ret.push(e); });
+ if (ret.empty()) {
+ ret.note("Error while loading body factory templates");
+ }
+ ret.note(r);
Review Comment:
same
##########
mgmt/config/FileManager.cc:
##########
@@ -226,7 +227,10 @@ FileManager::rereadConfig()
auto const &r = fileChanged(rb->getFileName(), rb->getConfigName());
if (!r) {
- std::for_each(r.begin(), r.end(), [&ret](auto &&e) { ret.push(e); });
+ if (ret.empty()) {
+ ret.note("Errors while reloading configurations.");
+ }
+ ret.note(r);
Review Comment:
same here, just:
```cpp
ret.note(r);
```
##########
mgmt/config/FileManager.cc:
##########
@@ -266,7 +270,10 @@ FileManager::rereadConfig()
for (size_t i = 0; i < n; i++) {
if (std::find(changedFiles.begin(), changedFiles.end(),
parentFileNeedChange[i]) == changedFiles.end()) {
if (auto const &r = fileChanged(parentFileNeedChange[i]->getFileName(),
parentFileNeedChange[i]->getConfigName()); !r) {
- std::for_each(r.begin(), r.end(), [&ret](auto &&e) { ret.push(e); });
+ if (ret.empty()) {
+ ret.note("Error while handling parent file name changed.");
+ }
+ ret.note(r);
Review Comment:
same
##########
mgmt/rpc/handlers/common/ErrorUtils.h:
##########
@@ -24,7 +24,7 @@
#include <system_error>
#include <string_view>
-#include "tscore/Errata.h"
+#include "tscpp//util/ts_errata.h"
Review Comment:
double `/` .
##########
mgmt/rpc/handlers/common/ErrorUtils.h:
##########
@@ -37,7 +37,7 @@ namespace rpc::handlers::errors
// With this we try to avoid error codes collision. You can also use same
error Code for all your
// errors.
enum Codes : unsigned int {
- CONFIGURATION = 1,
+ CONFIGURATION = 999, // go past @c errno
Review Comment:
Although the codes design is not the best, the idea was to have some range
between each handler logic, if config starts in `999`, there will clash with
the next one which starts at `1000`.
We can probably increase them all giving some `range` between them:
```cpp
enum Codes : unsigned int {
CONFIGURATION = 1000, // go past @c errno
METRIC = 10000,
RECORD = 20000,
SERVER = 30000,
STORAGE = 40000,
PLUGIN = 50000,
GENERIC = 300000
};
```
##########
mgmt/rpc/jsonrpc/JsonRPCManager.cc:
##########
@@ -57,15 +57,15 @@ RPCRegistryInfo core_ats_rpc_service_provider_handle = {
// plugin rpc handling variables.
std::mutex g_rpcHandlingMutex;
std::condition_variable g_rpcHandlingCompletion;
-ts::Rv<YAML::Node> g_rpcHandlerResponseData;
+swoc::Rv<YAML::Node> g_rpcHandlerResponseData;
bool g_rpcHandlerProcessingCompleted{false};
// --- Helpers
-std::pair<ts::Errata, error::RPCErrorCode>
+swoc::Errata
check_for_blockers(Context const &ctx, TSRPCHandlerOptions const &options)
{
- if (auto err = ctx.get_auth().is_blocked(options); !err.isOK()) {
- return {err, error::RPCErrorCode::Unauthorized};
+ if (auto err = ctx.get_auth().is_blocked(options); !err.is_ok()) {
+ return
std::move(err.assign(std::error_code(unsigned(error::RPCErrorCode::Unauthorized),
std::generic_category())));
Review Comment:
`make_error_code` is supported for `RPCErrorCode`:
```cpp
return
std::move(err.assign(error::make_error_code(error::RPCErrorCode::Unauthorized)));
```
##########
mgmt/rpc/handlers/server/Server.cc:
##########
@@ -90,23 +90,26 @@ server_start_drain(std::string_view const &id, YAML::Node
const ¶ms)
if (!is_server_draining()) {
set_server_drain(true);
} else {
- resp.errata().push(err::make_errata(err::Codes::SERVER, "Server already
draining."));
+ resp.errata().assign(std::error_code(unsigned(err::Codes::SERVER),
std::generic_category()));
+ resp.errata().note("Server already draining.");
Review Comment:
I have not implemented `std::error_code` support for the generic codes
though, I can do that after you commit this and simplify this two lines.
##########
mgmt/rpc/jsonrpc/json/YAMLCodec.h:
##########
@@ -174,41 +174,31 @@ class yamlcpp_json_encoder
{
///
/// @brief Function to encode an error.
- /// Error could be from two sources, presence of @c std::error_code means a
high level and the @c ts::Errata a callee . Both will
- /// be written into the passed @c out YAML::Emitter. This is mainly a
convenience class for the other two encode_* functions.
+ /// Error could be from two sources, presence of @c std::error_code means a
high level and the @c swoc::Errata a callee . Both
+ /// will be written into the passed @c out YAML::Emitter. This is mainly a
convenience class for the other two encode_* functions.
///
/// @param error std::error_code High level, main error.
/// @param errata the Errata from the callee
/// @param json output parameter. YAML::Emitter.
///
static void
- encode_error(std::error_code error, ts::Errata const &errata, YAML::Emitter
&json)
+ encode_error(swoc::Errata const &errata, YAML::Emitter &json)
{
json << YAML::Key << "error";
json << YAML::BeginMap;
- json << YAML::Key << "code" << YAML::Value << error.value();
- json << YAML::Key << "message" << YAML::Value << error.message();
- if (!errata.isOK()) {
+ json << YAML::Key << "code" << YAML::Value << errata.code().value();
+ json << YAML::Key << "message" << YAML::Value << errata.code().message();
+ if (!errata.is_ok()) {
json << YAML::Key << "data";
json << YAML::BeginSeq;
for (auto const &err : errata) {
- json << YAML::BeginMap;
- json << YAML::Key << "code" << YAML::Value << err.getCode();
- json << YAML::Key << "message" << YAML::Value << err.text();
- json << YAML::EndMap;
+ json << YAML::Value << std::string(err.text()); // FIXME!! Shouldn't
need a @c std::string
Review Comment:
This is an important change, we need `code` and `message` as part of the
aditional `data` message information.
##########
mgmt/rpc/jsonrpc/JsonRPCManager.cc:
##########
@@ -154,17 +155,17 @@
JsonRPCManager::Dispatcher::invoke_method_handler(JsonRPCManager::Dispatcher::In
specs::RPCResponseInfo response{request.id};
try {
- auto const &rv = handler.invoke(request);
+ auto rv = handler.invoke(request);
- if (rv.isOK()) {
+ if (rv.is_ok()) {
response.callResult.result = rv.result();
} else {
// if we have some errors to log, then include it.
- response.callResult.errata = rv.errata();
+ response.callResult.errata = std::move(rv.errata());
}
} catch (std::exception const &e) {
Debug(logTag, "Oops, something happened during the callback invocation:
%s", e.what());
- response.error.ec = error::RPCErrorCode::ExecutionError;
+
response.error.assign(std::error_code(unsigned(error::RPCErrorCode::ExecutionError),
std::generic_category()));
Review Comment:
```cpp
response.error.assign(error::make_error_code(error::RPCErrorCode::ExecutionError));
```
##########
mgmt/rpc/handlers/records/Records.cc:
##########
@@ -247,19 +247,20 @@ lookup_records(std::string_view const &id, YAML::Node
const ¶ms)
return resp;
}
-ts::Rv<YAML::Node>
+swoc::Rv<YAML::Node>
clear_all_metrics_records(std::string_view const &id, YAML::Node const ¶ms)
{
using namespace rpc::handlers::records::utils;
- ts::Rv<YAML::Node> resp;
+ swoc::Rv<YAML::Node> resp;
if (RecResetStatRecord(RECT_NULL, true) != REC_ERR_OKAY) {
- return ts::Errata{rpc::handlers::errors::RecordError::RECORD_WRITE_ERROR};
+ return
{swoc::Errata(std::error_code{unsigned(rpc::handlers::errors::RecordError::RECORD_WRITE_ERROR),
std::generic_category()},
Review Comment:
I have implemented support `std::error_code` in `RecordError`, we can just
call `make_error_code`.
```cpp
if (RecResetStatRecord(RECT_NULL, true) != REC_ERR_OKAY) {
namespace err = rpc::handlers::errors;
resp.note(err::make_error_code(err::RecordError::RECORD_WRITE_ERROR));
}
```
return resp;
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
