You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2022/02/21 18:34:15 UTC
[GitHub] [trafficserver] jrushford opened a new pull request #8689: WIP: Hoststatus remove stats creation.
jrushford opened a new pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689
Work in progress to remove stats creation from HostStatus.
--
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] jrushford commented on pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1064210240
> @jrushford this looks good, just have a question.
> Do you think you will be injectingthe `host_records.yaml` into traffic_server through the JSON-RPC node?(by using traffic_ctl or any other tool). If this is the plan at some point maybe the field names in the rpc request and the dump `host_records.yaml` file should be the same in order to re-use them.
@brbzull0 I didn't have any plans to use the JSON-RPC node to inject them but, that could be useful. In this PR, the host_records.yaml is loaded at startup by trafficserver. However, I'll take a look and make sure that the names are the same, someone might have a use case for the injection.
--
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] jrushford commented on pull request #8689: WIP: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1058804695
[approve ci clang-analyzer]
--
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 #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1064202803
@jrushford this looks good, just have a question.
Do you think you will be injectingthe `host_records.yaml` into traffic_server through the JSON-RPC node?(by using traffic_ctl or any other tool). If this is the plan at some point maybe the field names in the rpc request and the dump `host_records.yaml` file should be the same in order to re-use them.
--
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] jrushford commented on a change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r827394303
##########
File path: proxy/HostStatus.h
##########
@@ -168,6 +175,35 @@ struct HostStatRec {
}
};
+struct HostStatusSync;
+typedef int (HostStatusSync::*HostStatusSyncHandler)(int, void *);
Review comment:
@ywkaras ok, I removed it.
--
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] ywkaras commented on a change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r827296889
##########
File path: doc/appendices/command-line/traffic_ctl_jsonrpc.en.rst
##########
@@ -366,7 +365,8 @@ endpoint.
:ref:`admin_lookup_records`
Get the current status of the specified hosts with respect to their use as targets for parent
- selection. This returns the same information as the per host stat.
+ selection. This returns the same information as the per host record. If the HOSTNAME arguments
Review comment:
But the status subcommand is explained in line 328. It seems like what this is saying is that the host record returns the same information as the host record, so maybe we can just delete that sentence.
--
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 #8689: WIP: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1048160709
[approve ci autest]
--
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] jrushford commented on a change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r818789840
##########
File path: src/traffic_server/traffic_server.cc
##########
@@ -2162,7 +2162,6 @@ main(int /* argc ATS_UNUSED */, const char **argv)
RecProcessStart();
initCacheControl();
IpAllow::startup();
- HostStatus::instance().loadHostStatusFromStats();
Review comment:
@brbzull0 when the parent.config or strategies.yaml files are loaded, they call HostStatus::instance(). However, I'll add HostStatus::instance() back in.
##########
File path: doc/appendices/command-line/traffic_ctl_jsonrpc.en.rst
##########
@@ -366,7 +365,8 @@ endpoint.
:ref:`admin_lookup_records`
Get the current status of the specified hosts with respect to their use as targets for parent
- selection. This returns the same information as the per host stat.
+ selection. This returns the same information as the per host record. If the HOSTNAME arguments
Review comment:
I'm not sure what your asking? Do you want a change?
--
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] jrushford commented on pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1070978126
@brbzull0 and @ywkaras any more changes? Are you ok with the `const &` removal?
--
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 #8689: WIP: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1047248147
[approve ci rocky]
--
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] jrushford commented on pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1068490577
[approve ci fedora]
--
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 change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r827912983
##########
File path: src/traffic_server/HostStatus.cc
##########
@@ -44,8 +44,8 @@ struct HostCmdInfo {
} // namespace
-ts::Rv<YAML::Node> server_get_status(std::string_view const &id, YAML::Node const ¶ms);
-ts::Rv<YAML::Node> server_set_status(std::string_view const &id, YAML::Node const ¶ms);
+ts::Rv<YAML::Node> server_get_status(std::string_view const id, YAML::Node const ¶ms);
+ts::Rv<YAML::Node> server_set_status(std::string_view const id, YAML::Node const ¶ms);
Review comment:
> It's redundant to use 'string_view const' instead of just plain 'string_view' but harmless.
Totally, but the reason is this:
https://github.com/apache/trafficserver/blob/68e69e27b87576abfaa7a1bbeb8394795c522e82/mgmt2/rpc/jsonrpc/JsonRPCManager.h#L64
It is expected to be like this.
--
The reason is `const&` is becase I wanted to make it clear that this should not mutate despite the fact that you cannot change it if it was only a `std::string_view`. I always thought that this is ambiguous and if someone try to change it well, good luck. Now kind of I am inclined to remove the `const &`
--
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] ywkaras commented on a change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r827397919
##########
File path: src/traffic_server/HostStatus.cc
##########
@@ -44,8 +44,8 @@ struct HostCmdInfo {
} // namespace
-ts::Rv<YAML::Node> server_get_status(std::string_view const &id, YAML::Node const ¶ms);
-ts::Rv<YAML::Node> server_set_status(std::string_view const &id, YAML::Node const ¶ms);
+ts::Rv<YAML::Node> server_get_status(std::string_view const id, YAML::Node const ¶ms);
+ts::Rv<YAML::Node> server_set_status(std::string_view const id, YAML::Node const ¶ms);
Review comment:
It's redundant to use 'string_view const' instead of just plain 'string_view' but harmless.
--
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] jrushford commented on a change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r827410234
##########
File path: src/traffic_server/HostStatus.cc
##########
@@ -44,8 +44,8 @@ struct HostCmdInfo {
} // namespace
-ts::Rv<YAML::Node> server_get_status(std::string_view const &id, YAML::Node const ¶ms);
-ts::Rv<YAML::Node> server_set_status(std::string_view const &id, YAML::Node const ¶ms);
+ts::Rv<YAML::Node> server_get_status(std::string_view const id, YAML::Node const ¶ms);
+ts::Rv<YAML::Node> server_set_status(std::string_view const id, YAML::Node const ¶ms);
Review comment:
@ywkaras I'll change it.
--
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] jrushford commented on a change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r827393385
##########
File path: proxy/HostStatus.h
##########
@@ -182,11 +218,16 @@ struct HostStatus {
}
void setHostStatus(const std::string_view name, const TSHostStatus status, const unsigned int down_time,
const unsigned int reason);
- HostStatRec *getHostStatus(const std::string_view name);
- void createHostStat(const std::string_view name, const char *data = nullptr);
- void loadHostStatusFromStats();
+ void loadFromPersistentStore();
void loadRecord(std::string_view name, HostStatRec &h);
- RecErrT getHostStat(std::string &stat_name, char *buf, unsigned int buf_len);
+ HostStatRec *getHostStatus(const std::string_view name);
+ void getAllHostStatuses(std::vector<HostStatuses> &hosts);
Review comment:
@ywkaras I could but this isn't wrong. Besides, if the singleton unordered_map is empty, I just return early.
--
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 #8689: WIP: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1047879812
https://github.com/apache/trafficserver/pull/8694 should fix the autest. Once it's merged into 10-Dev you can rebase.
--
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 #8689: WIP: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1056004141
[approve ci autest]
--
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 change in pull request #8689: WIP: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r818571958
##########
File path: src/traffic_server/traffic_server.cc
##########
@@ -2162,7 +2162,6 @@ main(int /* argc ATS_UNUSED */, const char **argv)
RecProcessStart();
initCacheControl();
IpAllow::startup();
- HostStatus::instance().loadHostStatusFromStats();
Review comment:
This was key I think, it was creating the first `HostStatus` instance, without this the ctor will not be called so no handlers registered. You should probably need a `init()` function to register the handlers.
--
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 #8689: WIP: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1048925845
[approve ci fedora]
--
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 removed a comment on pull request #8689: WIP: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
bneradt removed a comment on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1047248147
[approve ci rocky]
--
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] jrushford commented on pull request #8689: WIP: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1058804311
[approve ci clang-analyzer
--
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] jrushford removed a comment on pull request #8689: WIP: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford removed a comment on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1058386279
[approve ci Fedora]
--
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] jrushford commented on a change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r827394845
##########
File path: proxy/HostStatus.h
##########
@@ -168,6 +175,35 @@ struct HostStatRec {
}
};
+struct HostStatusSync;
+typedef int (HostStatusSync::*HostStatusSyncHandler)(int, void *);
+struct HostStatusSync : public Continuation {
+ std::string hostRecordsFile;
+
+ void sync_task();
+ void
+ getHostStatusPersistentFilePath()
+ {
+ std::string rundir(RecConfigReadRuntimeDir());
+ hostRecordsFile = Layout::relative_to(rundir, ts::filename::HOST_RECORDS);
+ }
+
+public:
+ HostStatusSync()
+ {
+ getHostStatusPersistentFilePath();
+
+ SET_HANDLER((HostStatusSyncHandler)&HostStatusSync::mainEvent);
Review comment:
@ywkaras ok, it's removed.
--
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 #8689: WIP: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1047254379
[approve ci rocky]
--
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] jrushford commented on a change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r827325326
##########
File path: doc/appendices/command-line/traffic_ctl_jsonrpc.en.rst
##########
@@ -366,7 +365,8 @@ endpoint.
:ref:`admin_lookup_records`
Get the current status of the specified hosts with respect to their use as targets for parent
- selection. This returns the same information as the per host stat.
+ selection. This returns the same information as the per host record. If the HOSTNAME arguments
Review comment:
ahh ok, I'll do that
--
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] ywkaras commented on a change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r827262017
##########
File path: proxy/HostStatus.h
##########
@@ -182,11 +218,16 @@ struct HostStatus {
}
void setHostStatus(const std::string_view name, const TSHostStatus status, const unsigned int down_time,
const unsigned int reason);
- HostStatRec *getHostStatus(const std::string_view name);
- void createHostStat(const std::string_view name, const char *data = nullptr);
- void loadHostStatusFromStats();
+ void loadFromPersistentStore();
void loadRecord(std::string_view name, HostStatRec &h);
- RecErrT getHostStat(std::string &stat_name, char *buf, unsigned int buf_len);
+ HostStatRec *getHostStatus(const std::string_view name);
+ void getAllHostStatuses(std::vector<HostStatuses> &hosts);
Review comment:
You could return a vector, it should be efficient due to Return Value Optimization.
--
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 change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r827910733
##########
File path: src/traffic_server/HostStatus.cc
##########
@@ -20,85 +20,32 @@
See the License for the specific language governing permissions and
limitations under the License.
*/
+#include <fstream>
#include "HostStatus.h"
#include "ProcessManager.h"
#include "tscore/BufferWriter.h"
#include "rpc/jsonrpc/JsonRPC.h"
+#include "shared/rpc/RPCRequests.h"
-ts::Rv<YAML::Node> server_set_status(std::string_view const &id, YAML::Node const ¶ms);
-
-inline void
-getStatName(std::string &stat_name, const std::string_view name)
-{
- stat_name.clear();
- stat_name.append(stat_prefix).append(name);
-}
-
-static void
-mgmt_host_status_up_callback(ts::MemSpan<void> span)
+namespace
{
- MgmtInt op;
- MgmtMarshallString name;
- MgmtMarshallInt down_time;
- MgmtMarshallString reason_str;
- std::string stat_name;
- char buf[1024] = {0};
- char *data = static_cast<char *>(span.data());
- auto len = span.size();
- static const MgmtMarshallType fields[] = {MGMT_MARSHALL_INT, MGMT_MARSHALL_STRING, MGMT_MARSHALL_STRING, MGMT_MARSHALL_INT};
- Debug("host_statuses", "%s:%s:%d - data: %s, len: %ld\n", __FILE__, __func__, __LINE__, data, len);
-
- if (mgmt_message_parse(data, len, fields, countof(fields), &op, &name, &reason_str, &down_time) == -1) {
- Error("Plugin message - RPC parsing error - message discarded.");
- }
- Debug("host_statuses", "op: %ld, name: %s, down_time: %d, reason_str: %s", static_cast<long>(op), name,
- static_cast<int>(down_time), reason_str);
+const std::string STATUS_LIST_KEY{"statusList"};
+const std::string ERROR_LIST_KEY{"errorList"};
+const std::string HOST_NAME_KEY{"hostname"};
+const std::string STATUS_KEY{"status"};
- unsigned int reason = Reason::getReason(reason_str);
-
- getStatName(stat_name, name);
- if (data != nullptr) {
- Debug("host_statuses", "marking up server %s", data);
- HostStatus &hs = HostStatus::instance();
- if (hs.getHostStat(stat_name, buf, 1024) == REC_ERR_FAIL) {
- hs.createHostStat(name);
- }
- hs.setHostStatus(name, TSHostStatus::TS_HOST_STATUS_UP, down_time, reason);
- }
-}
-
-static void
-mgmt_host_status_down_callback(ts::MemSpan<void> span)
-{
- MgmtInt op;
- MgmtMarshallString name;
- MgmtMarshallInt down_time;
- MgmtMarshallString reason_str;
- std::string stat_name;
- char *data = static_cast<char *>(span.data());
- char buf[1024] = {0};
- auto len = span.size();
- static const MgmtMarshallType fields[] = {MGMT_MARSHALL_INT, MGMT_MARSHALL_STRING, MGMT_MARSHALL_STRING, MGMT_MARSHALL_INT};
- Debug("host_statuses", "%s:%s:%d - data: %s, len: %ld\n", __FILE__, __func__, __LINE__, data, len);
-
- if (mgmt_message_parse(data, len, fields, countof(fields), &op, &name, &reason_str, &down_time) == -1) {
- Error("Plugin message - RPC parsing error - message discarded.");
- }
- Debug("host_statuses", "op: %ld, name: %s, down_time: %d, reason_str: %s", static_cast<long>(op), name,
- static_cast<int>(down_time), reason_str);
+struct HostCmdInfo {
+ TSHostStatus type{TSHostStatus::TS_HOST_STATUS_INIT};
+ unsigned int reasonType{0};
+ std::vector<std::string> hosts;
+ int time{0};
+};
- unsigned int reason = Reason::getReason(reason_str);
+} // namespace
- if (data != nullptr) {
- Debug("host_statuses", "marking down server %s", name);
- HostStatus &hs = HostStatus::instance();
- if (hs.getHostStat(stat_name, buf, 1024) == REC_ERR_FAIL) {
- hs.createHostStat(name);
- }
- hs.setHostStatus(name, TSHostStatus::TS_HOST_STATUS_DOWN, down_time, reason);
- }
-}
+ts::Rv<YAML::Node> server_get_status(std::string_view const &id, YAML::Node const ¶ms);
+ts::Rv<YAML::Node> server_set_status(std::string_view const &id, YAML::Node const ¶ms);
Review comment:
> Why reference to string_view ?
It is expected to be like this:
https://github.com/apache/trafficserver/blob/68e69e27b87576abfaa7a1bbeb8394795c522e82/mgmt2/rpc/jsonrpc/JsonRPCManager.h#L64
The reason is `const&` is becase I wanted to make it clear that this should not mutate despite the fact that you cannot change it if it was only a `std::string_view`. I always thought that this is ambiguous and if someone try to change it well, good luck. Now kind of I am inclined to remove the `const &`
--
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] jrushford merged pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford merged pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689
--
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] jrushford commented on pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1059461030
[approve ci fedora]
--
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 removed a comment on pull request #8689: WIP: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
bneradt removed a comment on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1056004141
[approve ci autest]
--
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] jrushford commented on pull request #8689: WIP: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1058625092
@brbzull0 I've added in the persistence. You'll see that I added back in the HostStatus call in traffic_server.cc with the new HostStatus::loadFromPersistentStore() function
--
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] ywkaras commented on a change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r827254019
##########
File path: proxy/HostStatus.h
##########
@@ -168,6 +175,35 @@ struct HostStatRec {
}
};
+struct HostStatusSync;
+typedef int (HostStatusSync::*HostStatusSyncHandler)(int, void *);
Review comment:
Don't need this.
--
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] ywkaras commented on pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1068461105
Looks good to me, let's see what Damian says.
--
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] jrushford commented on a change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r827395647
##########
File path: src/traffic_server/HostStatus.cc
##########
@@ -20,85 +20,32 @@
See the License for the specific language governing permissions and
limitations under the License.
*/
+#include <fstream>
#include "HostStatus.h"
#include "ProcessManager.h"
#include "tscore/BufferWriter.h"
#include "rpc/jsonrpc/JsonRPC.h"
+#include "shared/rpc/RPCRequests.h"
-ts::Rv<YAML::Node> server_set_status(std::string_view const &id, YAML::Node const ¶ms);
-
-inline void
-getStatName(std::string &stat_name, const std::string_view name)
-{
- stat_name.clear();
- stat_name.append(stat_prefix).append(name);
-}
-
-static void
-mgmt_host_status_up_callback(ts::MemSpan<void> span)
+namespace
{
- MgmtInt op;
- MgmtMarshallString name;
- MgmtMarshallInt down_time;
- MgmtMarshallString reason_str;
- std::string stat_name;
- char buf[1024] = {0};
- char *data = static_cast<char *>(span.data());
- auto len = span.size();
- static const MgmtMarshallType fields[] = {MGMT_MARSHALL_INT, MGMT_MARSHALL_STRING, MGMT_MARSHALL_STRING, MGMT_MARSHALL_INT};
- Debug("host_statuses", "%s:%s:%d - data: %s, len: %ld\n", __FILE__, __func__, __LINE__, data, len);
-
- if (mgmt_message_parse(data, len, fields, countof(fields), &op, &name, &reason_str, &down_time) == -1) {
- Error("Plugin message - RPC parsing error - message discarded.");
- }
- Debug("host_statuses", "op: %ld, name: %s, down_time: %d, reason_str: %s", static_cast<long>(op), name,
- static_cast<int>(down_time), reason_str);
+const std::string STATUS_LIST_KEY{"statusList"};
+const std::string ERROR_LIST_KEY{"errorList"};
+const std::string HOST_NAME_KEY{"hostname"};
+const std::string STATUS_KEY{"status"};
- unsigned int reason = Reason::getReason(reason_str);
-
- getStatName(stat_name, name);
- if (data != nullptr) {
- Debug("host_statuses", "marking up server %s", data);
- HostStatus &hs = HostStatus::instance();
- if (hs.getHostStat(stat_name, buf, 1024) == REC_ERR_FAIL) {
- hs.createHostStat(name);
- }
- hs.setHostStatus(name, TSHostStatus::TS_HOST_STATUS_UP, down_time, reason);
- }
-}
-
-static void
-mgmt_host_status_down_callback(ts::MemSpan<void> span)
-{
- MgmtInt op;
- MgmtMarshallString name;
- MgmtMarshallInt down_time;
- MgmtMarshallString reason_str;
- std::string stat_name;
- char *data = static_cast<char *>(span.data());
- char buf[1024] = {0};
- auto len = span.size();
- static const MgmtMarshallType fields[] = {MGMT_MARSHALL_INT, MGMT_MARSHALL_STRING, MGMT_MARSHALL_STRING, MGMT_MARSHALL_INT};
- Debug("host_statuses", "%s:%s:%d - data: %s, len: %ld\n", __FILE__, __func__, __LINE__, data, len);
-
- if (mgmt_message_parse(data, len, fields, countof(fields), &op, &name, &reason_str, &down_time) == -1) {
- Error("Plugin message - RPC parsing error - message discarded.");
- }
- Debug("host_statuses", "op: %ld, name: %s, down_time: %d, reason_str: %s", static_cast<long>(op), name,
- static_cast<int>(down_time), reason_str);
+struct HostCmdInfo {
+ TSHostStatus type{TSHostStatus::TS_HOST_STATUS_INIT};
+ unsigned int reasonType{0};
+ std::vector<std::string> hosts;
+ int time{0};
+};
- unsigned int reason = Reason::getReason(reason_str);
+} // namespace
- if (data != nullptr) {
- Debug("host_statuses", "marking down server %s", name);
- HostStatus &hs = HostStatus::instance();
- if (hs.getHostStat(stat_name, buf, 1024) == REC_ERR_FAIL) {
- hs.createHostStat(name);
- }
- hs.setHostStatus(name, TSHostStatus::TS_HOST_STATUS_DOWN, down_time, reason);
- }
-}
+ts::Rv<YAML::Node> server_get_status(std::string_view const &id, YAML::Node const ¶ms);
+ts::Rv<YAML::Node> server_set_status(std::string_view const &id, YAML::Node const ¶ms);
Review comment:
@ywkaras okay, will pass by value.
--
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] jrushford commented on a change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r827394575
##########
File path: doc/appendices/command-line/traffic_ctl_jsonrpc.en.rst
##########
@@ -366,7 +365,8 @@ endpoint.
:ref:`admin_lookup_records`
Get the current status of the specified hosts with respect to their use as targets for parent
- selection. This returns the same information as the per host stat.
+ selection. This returns the same information as the per host record. If the HOSTNAME arguments
Review comment:
@ywkaras ok, it's removed.
--
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 removed a comment on pull request #8689: WIP: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
bneradt removed a comment on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1048160709
[approve ci autest]
--
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 removed a comment on pull request #8689: WIP: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
bneradt removed a comment on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1048925845
[approve ci fedora]
--
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] jrushford commented on pull request #8689: WIP: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1048051802
[approve ci autest]
--
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] ywkaras commented on a change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r827257257
##########
File path: proxy/HostStatus.h
##########
@@ -168,6 +175,35 @@ struct HostStatRec {
}
};
+struct HostStatusSync;
+typedef int (HostStatusSync::*HostStatusSyncHandler)(int, void *);
+struct HostStatusSync : public Continuation {
+ std::string hostRecordsFile;
+
+ void sync_task();
+ void
+ getHostStatusPersistentFilePath()
+ {
+ std::string rundir(RecConfigReadRuntimeDir());
+ hostRecordsFile = Layout::relative_to(rundir, ts::filename::HOST_RECORDS);
+ }
+
+public:
+ HostStatusSync()
+ {
+ getHostStatusPersistentFilePath();
+
+ SET_HANDLER((HostStatusSyncHandler)&HostStatusSync::mainEvent);
Review comment:
You should not need to cast the type of the event function anymore because of https://github.com/apache/trafficserver/pull/6996 .
It is best to not use C casts in new code. A C cast is equivalent to reinterpret_cast and is hard to find with grep.
--
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] jrushford commented on pull request #8689: WIP: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1058386758
[approve ci fedora]
--
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] jrushford commented on pull request #8689: WIP: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1058386279
[approve ci Fedora]
--
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] ywkaras commented on a change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r827283355
##########
File path: src/traffic_ctl_jsonrpc/jsonrpc/CtrlRPCRequests.h
##########
@@ -129,10 +139,16 @@ struct HostSetStatusRequest : shared::rpc::ClientRequest {
}
};
-struct HostGetStatusRequest : shared::rpc::RecordLookupRequest {
- static constexpr auto STATUS_PREFIX = "proxy.process.host_status";
- using super = shared::rpc::RecordLookupRequest;
- HostGetStatusRequest() : super() {}
+struct HostGetStatusRequest : shared::rpc::ClientRequest {
+ using super = shared::rpc::ClientRequest;
+ using Params = std::vector<std::string>;
+ HostGetStatusRequest(Params p) { super::params = std::move(p); }
+
+ std::string
+ get_method() const
+ {
Review comment:
@brbzull0 it's not clear why this even needs to be a virtual function: https://github.com/apache/trafficserver/blob/68e69e27b87576abfaa7a1bbeb8394795c522e82/include/shared/rpc/RPCRequests.h#L45
Should derived classes just set the "method" member string?
--
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 change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r827910733
##########
File path: src/traffic_server/HostStatus.cc
##########
@@ -20,85 +20,32 @@
See the License for the specific language governing permissions and
limitations under the License.
*/
+#include <fstream>
#include "HostStatus.h"
#include "ProcessManager.h"
#include "tscore/BufferWriter.h"
#include "rpc/jsonrpc/JsonRPC.h"
+#include "shared/rpc/RPCRequests.h"
-ts::Rv<YAML::Node> server_set_status(std::string_view const &id, YAML::Node const ¶ms);
-
-inline void
-getStatName(std::string &stat_name, const std::string_view name)
-{
- stat_name.clear();
- stat_name.append(stat_prefix).append(name);
-}
-
-static void
-mgmt_host_status_up_callback(ts::MemSpan<void> span)
+namespace
{
- MgmtInt op;
- MgmtMarshallString name;
- MgmtMarshallInt down_time;
- MgmtMarshallString reason_str;
- std::string stat_name;
- char buf[1024] = {0};
- char *data = static_cast<char *>(span.data());
- auto len = span.size();
- static const MgmtMarshallType fields[] = {MGMT_MARSHALL_INT, MGMT_MARSHALL_STRING, MGMT_MARSHALL_STRING, MGMT_MARSHALL_INT};
- Debug("host_statuses", "%s:%s:%d - data: %s, len: %ld\n", __FILE__, __func__, __LINE__, data, len);
-
- if (mgmt_message_parse(data, len, fields, countof(fields), &op, &name, &reason_str, &down_time) == -1) {
- Error("Plugin message - RPC parsing error - message discarded.");
- }
- Debug("host_statuses", "op: %ld, name: %s, down_time: %d, reason_str: %s", static_cast<long>(op), name,
- static_cast<int>(down_time), reason_str);
+const std::string STATUS_LIST_KEY{"statusList"};
+const std::string ERROR_LIST_KEY{"errorList"};
+const std::string HOST_NAME_KEY{"hostname"};
+const std::string STATUS_KEY{"status"};
- unsigned int reason = Reason::getReason(reason_str);
-
- getStatName(stat_name, name);
- if (data != nullptr) {
- Debug("host_statuses", "marking up server %s", data);
- HostStatus &hs = HostStatus::instance();
- if (hs.getHostStat(stat_name, buf, 1024) == REC_ERR_FAIL) {
- hs.createHostStat(name);
- }
- hs.setHostStatus(name, TSHostStatus::TS_HOST_STATUS_UP, down_time, reason);
- }
-}
-
-static void
-mgmt_host_status_down_callback(ts::MemSpan<void> span)
-{
- MgmtInt op;
- MgmtMarshallString name;
- MgmtMarshallInt down_time;
- MgmtMarshallString reason_str;
- std::string stat_name;
- char *data = static_cast<char *>(span.data());
- char buf[1024] = {0};
- auto len = span.size();
- static const MgmtMarshallType fields[] = {MGMT_MARSHALL_INT, MGMT_MARSHALL_STRING, MGMT_MARSHALL_STRING, MGMT_MARSHALL_INT};
- Debug("host_statuses", "%s:%s:%d - data: %s, len: %ld\n", __FILE__, __func__, __LINE__, data, len);
-
- if (mgmt_message_parse(data, len, fields, countof(fields), &op, &name, &reason_str, &down_time) == -1) {
- Error("Plugin message - RPC parsing error - message discarded.");
- }
- Debug("host_statuses", "op: %ld, name: %s, down_time: %d, reason_str: %s", static_cast<long>(op), name,
- static_cast<int>(down_time), reason_str);
+struct HostCmdInfo {
+ TSHostStatus type{TSHostStatus::TS_HOST_STATUS_INIT};
+ unsigned int reasonType{0};
+ std::vector<std::string> hosts;
+ int time{0};
+};
- unsigned int reason = Reason::getReason(reason_str);
+} // namespace
- if (data != nullptr) {
- Debug("host_statuses", "marking down server %s", name);
- HostStatus &hs = HostStatus::instance();
- if (hs.getHostStat(stat_name, buf, 1024) == REC_ERR_FAIL) {
- hs.createHostStat(name);
- }
- hs.setHostStatus(name, TSHostStatus::TS_HOST_STATUS_DOWN, down_time, reason);
- }
-}
+ts::Rv<YAML::Node> server_get_status(std::string_view const &id, YAML::Node const ¶ms);
+ts::Rv<YAML::Node> server_set_status(std::string_view const &id, YAML::Node const ¶ms);
Review comment:
This is the RPC handler API:
https://github.com/apache/trafficserver/blob/68e69e27b87576abfaa7a1bbeb8394795c522e82/mgmt2/rpc/jsonrpc/JsonRPCManager.h#L64
The reason is `const&` is becase I wanted to make it clear that this should not mutate despite the fact that you cannot change it if it was only a `std::string_view`. I always thought that this is ambiguous and if someone try to change it well, good luck. Now kind of I am inclined to remove the `const &`
--
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] jrushford commented on pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1068506485
[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] ywkaras commented on a change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r827238779
##########
File path: doc/appendices/command-line/traffic_ctl_jsonrpc.en.rst
##########
@@ -366,7 +365,8 @@ endpoint.
:ref:`admin_lookup_records`
Get the current status of the specified hosts with respect to their use as targets for parent
- selection. This returns the same information as the per host stat.
+ selection. This returns the same information as the per host record. If the HOSTNAME arguments
Review comment:
How do you see the per host record?
--
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] jrushford commented on pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1070978126
@brbzull0 and @ywkaras any more changes? Are you ok with the `const &` removal?
--
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] ywkaras commented on a change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r827402854
##########
File path: proxy/HostStatus.h
##########
@@ -182,11 +218,16 @@ struct HostStatus {
}
void setHostStatus(const std::string_view name, const TSHostStatus status, const unsigned int down_time,
const unsigned int reason);
- HostStatRec *getHostStatus(const std::string_view name);
- void createHostStat(const std::string_view name, const char *data = nullptr);
- void loadHostStatusFromStats();
+ void loadFromPersistentStore();
void loadRecord(std::string_view name, HostStatRec &h);
- RecErrT getHostStat(std::string &stat_name, char *buf, unsigned int buf_len);
+ HostStatRec *getHostStatus(const std::string_view name);
+ void getAllHostStatuses(std::vector<HostStatuses> &hosts);
Review comment:
We should let the people who went to grad school tell us if it's wrong.
--
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] jrushford commented on a change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r827274008
##########
File path: doc/appendices/command-line/traffic_ctl_jsonrpc.en.rst
##########
@@ -366,7 +365,8 @@ endpoint.
:ref:`admin_lookup_records`
Get the current status of the specified hosts with respect to their use as targets for parent
- selection. This returns the same information as the per host stat.
+ selection. This returns the same information as the per host record. If the HOSTNAME arguments
Review comment:
@ywkaras I'm not sure what your asking? Do you want a change? The command line would be
**traffic_ctl host status $the_fqdn_of_the_host**
--
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] jrushford commented on pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1068475616
[approve ci fedora]
--
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 change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r827899430
##########
File path: src/traffic_ctl_jsonrpc/jsonrpc/CtrlRPCRequests.h
##########
@@ -129,10 +139,16 @@ struct HostSetStatusRequest : shared::rpc::ClientRequest {
}
};
-struct HostGetStatusRequest : shared::rpc::RecordLookupRequest {
- static constexpr auto STATUS_PREFIX = "proxy.process.host_status";
- using super = shared::rpc::RecordLookupRequest;
- HostGetStatusRequest() : super() {}
+struct HostGetStatusRequest : shared::rpc::ClientRequest {
+ using super = shared::rpc::ClientRequest;
+ using Params = std::vector<std::string>;
+ HostGetStatusRequest(Params p) { super::params = std::move(p); }
+
+ std::string
+ get_method() const
+ {
Review comment:
> it's not clear why this even needs to be a virtual function
Because of this https://github.com/apache/trafficserver/blob/68e69e27b87576abfaa7a1bbeb8394795c522e82/include/shared/rpc/yaml_codecs.h#L211
>Should derived classes just set the "method" member string?
yes they can, in which case `JSONRPCRequest::get_method` will return it.
I wasn't very concern about the virtual thing in here as this is used by `traffic_ctl` and `traffic_top`.
--
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] ywkaras commented on a change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r827288448
##########
File path: src/traffic_server/HostStatus.cc
##########
@@ -20,85 +20,32 @@
See the License for the specific language governing permissions and
limitations under the License.
*/
+#include <fstream>
#include "HostStatus.h"
#include "ProcessManager.h"
#include "tscore/BufferWriter.h"
#include "rpc/jsonrpc/JsonRPC.h"
+#include "shared/rpc/RPCRequests.h"
-ts::Rv<YAML::Node> server_set_status(std::string_view const &id, YAML::Node const ¶ms);
-
-inline void
-getStatName(std::string &stat_name, const std::string_view name)
-{
- stat_name.clear();
- stat_name.append(stat_prefix).append(name);
-}
-
-static void
-mgmt_host_status_up_callback(ts::MemSpan<void> span)
+namespace
{
- MgmtInt op;
- MgmtMarshallString name;
- MgmtMarshallInt down_time;
- MgmtMarshallString reason_str;
- std::string stat_name;
- char buf[1024] = {0};
- char *data = static_cast<char *>(span.data());
- auto len = span.size();
- static const MgmtMarshallType fields[] = {MGMT_MARSHALL_INT, MGMT_MARSHALL_STRING, MGMT_MARSHALL_STRING, MGMT_MARSHALL_INT};
- Debug("host_statuses", "%s:%s:%d - data: %s, len: %ld\n", __FILE__, __func__, __LINE__, data, len);
-
- if (mgmt_message_parse(data, len, fields, countof(fields), &op, &name, &reason_str, &down_time) == -1) {
- Error("Plugin message - RPC parsing error - message discarded.");
- }
- Debug("host_statuses", "op: %ld, name: %s, down_time: %d, reason_str: %s", static_cast<long>(op), name,
- static_cast<int>(down_time), reason_str);
+const std::string STATUS_LIST_KEY{"statusList"};
+const std::string ERROR_LIST_KEY{"errorList"};
+const std::string HOST_NAME_KEY{"hostname"};
+const std::string STATUS_KEY{"status"};
- unsigned int reason = Reason::getReason(reason_str);
-
- getStatName(stat_name, name);
- if (data != nullptr) {
- Debug("host_statuses", "marking up server %s", data);
- HostStatus &hs = HostStatus::instance();
- if (hs.getHostStat(stat_name, buf, 1024) == REC_ERR_FAIL) {
- hs.createHostStat(name);
- }
- hs.setHostStatus(name, TSHostStatus::TS_HOST_STATUS_UP, down_time, reason);
- }
-}
-
-static void
-mgmt_host_status_down_callback(ts::MemSpan<void> span)
-{
- MgmtInt op;
- MgmtMarshallString name;
- MgmtMarshallInt down_time;
- MgmtMarshallString reason_str;
- std::string stat_name;
- char *data = static_cast<char *>(span.data());
- char buf[1024] = {0};
- auto len = span.size();
- static const MgmtMarshallType fields[] = {MGMT_MARSHALL_INT, MGMT_MARSHALL_STRING, MGMT_MARSHALL_STRING, MGMT_MARSHALL_INT};
- Debug("host_statuses", "%s:%s:%d - data: %s, len: %ld\n", __FILE__, __func__, __LINE__, data, len);
-
- if (mgmt_message_parse(data, len, fields, countof(fields), &op, &name, &reason_str, &down_time) == -1) {
- Error("Plugin message - RPC parsing error - message discarded.");
- }
- Debug("host_statuses", "op: %ld, name: %s, down_time: %d, reason_str: %s", static_cast<long>(op), name,
- static_cast<int>(down_time), reason_str);
+struct HostCmdInfo {
+ TSHostStatus type{TSHostStatus::TS_HOST_STATUS_INIT};
+ unsigned int reasonType{0};
+ std::vector<std::string> hosts;
+ int time{0};
+};
- unsigned int reason = Reason::getReason(reason_str);
+} // namespace
- if (data != nullptr) {
- Debug("host_statuses", "marking down server %s", name);
- HostStatus &hs = HostStatus::instance();
- if (hs.getHostStat(stat_name, buf, 1024) == REC_ERR_FAIL) {
- hs.createHostStat(name);
- }
- hs.setHostStatus(name, TSHostStatus::TS_HOST_STATUS_DOWN, down_time, reason);
- }
-}
+ts::Rv<YAML::Node> server_get_status(std::string_view const &id, YAML::Node const ¶ms);
+ts::Rv<YAML::Node> server_set_status(std::string_view const &id, YAML::Node const ¶ms);
Review comment:
Why reference to string_view ?
--
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] jrushford commented on a change in pull request #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford commented on a change in pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#discussion_r831477330
##########
File path: proxy/HostStatus.h
##########
@@ -168,6 +175,35 @@ struct HostStatRec {
}
};
+struct HostStatusSync;
+typedef int (HostStatusSync::*HostStatusSyncHandler)(int, void *);
Review comment:
changed
--
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 #8689: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1074384335
Looks good to me.
--
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] jrushford removed a comment on pull request #8689: WIP: Hoststatus remove stats creation.
Posted by GitBox <gi...@apache.org>.
jrushford removed a comment on pull request #8689:
URL: https://github.com/apache/trafficserver/pull/8689#issuecomment-1058804311
[approve ci clang-analyzer
--
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