You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "SolidWallOfCode (via GitHub)" <gi...@apache.org> on 2023/07/28 16:19:48 UTC
[GitHub] [trafficserver] SolidWallOfCode opened a new pull request, #10112: libsowc - remove legacy Errata.
SolidWallOfCode opened a new pull request, #10112:
URL: https://github.com/apache/trafficserver/pull/10112
I had to make some non-trivial changes in the JSON-RPC, @brbzull0 needs to review and, I expect, ask for some changes.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on pull request #10112: libsowc - remove legacy Errata.
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #10112:
URL: https://github.com/apache/trafficserver/pull/10112#issuecomment-1657918962
[approve ci]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #10112: libsowc - remove legacy Errata.
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] libsowc - remove legacy Errata. [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #10112:
URL: https://github.com/apache/trafficserver/pull/10112#issuecomment-1890432556
I will continue with this PR.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] libsowc - remove legacy Errata. [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 closed pull request #10112: libsowc - remove legacy Errata.
URL: https://github.com/apache/trafficserver/pull/10112
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #10112: libsowc - remove legacy Errata.
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #10112:
URL: https://github.com/apache/trafficserver/pull/10112#discussion_r1279405561
##########
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 disagree. The check provides additional context for the more local error, e.g. whether it happened during configuration loading, or key file loading, etc.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #10112: libsowc - remove legacy Errata.
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #10112:
URL: https://github.com/apache/trafficserver/pull/10112#discussion_r1279421390
##########
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 read those backwards. you are right, please ignore!
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #10112: libsowc - remove legacy Errata.
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #10112:
URL: https://github.com/apache/trafficserver/pull/10112#discussion_r1279397515
##########
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:
Hmmm. @bneradt made me take that out and overall I agree. We should also think about tracking the severity.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #10112: libsowc - remove legacy Errata.
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #10112:
URL: https://github.com/apache/trafficserver/pull/10112#discussion_r1279412511
##########
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:
The [data](https://www.jsonrpc.org/specification#error_object) is user defined information for the specific higher error, we use this structure to show inner error of an operation, like :
```json
{
"jsonrpc": "2.0",
"id": "msg001",
"error": {
"code": 9,
"message": "Error during execution",
"data": [{
"code": 3000,
"message": "Server already draining."
}]
}
}
```
We can include the severity inside each inner error object.
```json
"data": [{
"code": 3000,
"severity": "Fatal"
"message": "Server already draining."
}]
```
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #10112: libsowc - remove legacy Errata.
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #10112:
URL: https://github.com/apache/trafficserver/pull/10112#issuecomment-1659137743
[approve ci]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] libsowc - remove legacy Errata. [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #10112:
URL: https://github.com/apache/trafficserver/pull/10112#issuecomment-1891767562
Work continues here -> https://github.com/apache/trafficserver/pull/10986
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org