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 2021/02/01 21:59:37 UTC
[GitHub] [trafficserver] rob05c opened a new pull request #7467: WIP Parent Select Plugin
rob05c opened a new pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467
The goal of this is to do Parent Selection in a Plugin, to add the plugin hooks and funcs necessary to do parent selection, and to provide an example plugin with the intention of eventually replacing both parent.config and strategies.yaml in core, and completely removing them from core.
WIP Draft, still needs details handled, but it basically works.
To use it, create a strategies.yaml of the format in https://docs.trafficserver.apache.org/en/latest/admin-guide/files/strategies.yaml.en.html
And then on a the remap line add `@plugin=parent_select.so @pparam=etc/trafficserver/strategies.yaml @pparam=example-com` where the param is the name of the strategy.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on pull request #7467: WIP Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#issuecomment-772681548
@jrushford Ah, I missed adding those files. They're added now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#issuecomment-779967139
[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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#issuecomment-781457408
Yeah, I don't see any other uses in Core, except that `markParentUp`. The plugin still needs to know, I'll look into moving that into the plugin's self-contained `strategyTxn`. That'll be good, one less thing in core.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r576925898
##########
File path: include/ts/ts.h
##########
@@ -2641,6 +2648,27 @@ tsapi TSReturnCode TSHttpTxnClientStreamIdGet(TSHttpTxn txnp, uint64_t *stream_i
*/
tsapi TSReturnCode TSHttpTxnClientStreamPriorityGet(TSHttpTxn txnp, TSHttpPriority *priority);
+/*
+ * Returns TS_SUCCESS if hostname is this machine, as used for parent and remap self-detection.
+ * Returns TS_ERROR if hostname is not this machine.
+ */
+tsapi TSReturnCode TSHostnameIsSelf(const char *hostname);
Review comment:
Add a `length`, don't require a C-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r582077998
##########
File path: include/ts/nexthop.h
##########
@@ -28,22 +28,28 @@
#pragma once
-#include <ts/apidefs.h>
-
-// plugin callback commands.
-enum NHCmd { NH_MARK_UP, NH_MARK_DOWN };
-
-struct NHHealthStatus {
- virtual bool isNextHopAvailable(TSHttpTxn txn, const char *hostname, const int port, void *ih = nullptr) = 0;
- virtual void markNextHop(TSHttpTxn txn, const char *hostname, const int port, const NHCmd status, void *ih = nullptr,
- const time_t now = 0) = 0;
- virtual ~NHHealthStatus() {}
-};
-
-struct NHPluginStrategy {
- virtual void findNextHop(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual bool nextHopExists(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual ~NHPluginStrategy() {}
-
- NHHealthStatus *healthStatus;
-};
+// Plugins may set this to indicate how to retry.
+//
+// If handled is false, then no plugin set it, and Core will proceed to do its own thing.
+//
+// If handled is true, core will not do any parent processing, markdown, or anything else,
+// but will use the values in this for whether to use the existing response or make another request,
+// and what that request should look like.
+//
+// See the API functions which take this for ownership requirements of pointers, like hostname.
+//
+// DO NOT put non-POD data in this. It's passed by-copy from API calls.
+//
+typedef struct {
+ // TODO this shouldn't be necessary - plugins should manipulate the response as they see fit,
+ // core shouldn't "know" if it was a "success" or "failure," only the response or retry data/action.
+ // But for now, core needs to know, for reasons.
+ bool fail;
Review comment:
Changed bools to be adjacent
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r582079439
##########
File path: include/ts/ts.h
##########
@@ -2601,6 +2602,12 @@ tsapi TSReturnCode TSRemapFromUrlGet(TSHttpTxn txnp, TSMLoc *urlLocp);
//
tsapi TSReturnCode TSRemapToUrlGet(TSHttpTxn txnp, TSMLoc *urlLocp);
+// Override response behavior, and hard-set the state machine for whether to succeed or fail, and how.
+tsapi void TSHttpTxnResponseActionSet(TSHttpTxn txnp, TSResponseAction action);
Review comment:
Done
##########
File path: include/ts/ts.h
##########
@@ -2641,6 +2648,27 @@ tsapi TSReturnCode TSHttpTxnClientStreamIdGet(TSHttpTxn txnp, uint64_t *stream_i
*/
tsapi TSReturnCode TSHttpTxnClientStreamPriorityGet(TSHttpTxn txnp, TSHttpPriority *priority);
+/*
+ * Returns TS_SUCCESS if hostname is this machine, as used for parent and remap self-detection.
+ * Returns TS_ERROR if hostname is not this machine.
+ */
+tsapi TSReturnCode TSHostnameIsSelf(const char *hostname);
Review comment:
Done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r576938129
##########
File path: include/ts/ts.h
##########
@@ -30,6 +30,7 @@
#pragma once
#include <ts/apidefs.h>
+#include <ts/nexthop.h>
Review comment:
Sure. The name is a legacy, I think I'd prefer `parentselectdefs.h` or `parentdefs.h`. I'd like to standardize around "parent select" - we have too many names for the same thing X_X
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ezelkow1 removed a comment on pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
ezelkow1 removed a comment on pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#issuecomment-812070807
[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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r582079542
##########
File path: include/ts/ts.h
##########
@@ -2641,6 +2648,27 @@ tsapi TSReturnCode TSHttpTxnClientStreamIdGet(TSHttpTxn txnp, uint64_t *stream_i
*/
tsapi TSReturnCode TSHttpTxnClientStreamPriorityGet(TSHttpTxn txnp, TSHttpPriority *priority);
+/*
+ * Returns TS_SUCCESS if hostname is this machine, as used for parent and remap self-detection.
+ * Returns TS_ERROR if hostname is not this machine.
+ */
+tsapi TSReturnCode TSHostnameIsSelf(const char *hostname);
Review comment:
Done. Also added an overload to `HostStatus::instance().getHostStatus` to take a len, which this calls
##########
File path: include/ts/nexthop.h
##########
@@ -28,22 +28,28 @@
#pragma once
-#include <ts/apidefs.h>
-
-// plugin callback commands.
-enum NHCmd { NH_MARK_UP, NH_MARK_DOWN };
-
-struct NHHealthStatus {
- virtual bool isNextHopAvailable(TSHttpTxn txn, const char *hostname, const int port, void *ih = nullptr) = 0;
- virtual void markNextHop(TSHttpTxn txn, const char *hostname, const int port, const NHCmd status, void *ih = nullptr,
- const time_t now = 0) = 0;
- virtual ~NHHealthStatus() {}
-};
-
-struct NHPluginStrategy {
- virtual void findNextHop(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual bool nextHopExists(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual ~NHPluginStrategy() {}
-
- NHHealthStatus *healthStatus;
-};
+// Plugins may set this to indicate how to retry.
+//
+// If handled is false, then no plugin set it, and Core will proceed to do its own thing.
+//
+// If handled is true, core will not do any parent processing, markdown, or anything else,
+// but will use the values in this for whether to use the existing response or make another request,
+// and what that request should look like.
+//
+// See the API functions which take this for ownership requirements of pointers, like hostname.
+//
+// DO NOT put non-POD data in this. It's passed by-copy from API calls.
+//
+typedef struct {
+ // TODO this shouldn't be necessary - plugins should manipulate the response as they see fit,
+ // core shouldn't "know" if it was a "success" or "failure," only the response or retry data/action.
+ // But for now, core needs to know, for reasons.
+ bool fail;
+ const char *hostname;
+ int port;
+ bool retry; // whether this request is retrying a previously failed parent
Review comment:
Done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ezelkow1 merged pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
ezelkow1 merged pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#issuecomment-781443881
> The parent_result->retry is set when the retry window on a marked down parent elapses. In HttpTransact, if retry is true and the transaction is successful to the parent, HttpTransact will mark the parent up. That’s the only way markParentUp is ever called.
Hm, if markParent* is the only thing that's needed for, maybe it can be removed. The markParent* calls aren't done by Core for the Plugin, it does its own markdown entirely self-contained. I'll look over the code, to see if maybe that field can just be 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r576944218
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -178,12 +178,22 @@ inline static void
findParent(HttpTransact::State *s)
{
url_mapping *mp = s->url_map.getMapping();
-
- if (mp && mp->strategy) {
- return mp->strategy->findNextHop(reinterpret_cast<TSHttpTxn>(s->state_machine));
+ if (s->response_action.handled) {
+ s->parent_result.hostname = s->response_action.action.hostname;
+ s->parent_result.port = s->response_action.action.port;
+ s->parent_result.retry = s->response_action.action.retry;
Review comment:
I don't understand this value. Shouldn't the core determine / know if it's a retry? The plugin shouldn't be the authority on 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r580655037
##########
File path: include/ts/nexthop.h
##########
@@ -28,22 +28,28 @@
#pragma once
-#include <ts/apidefs.h>
-
-// plugin callback commands.
-enum NHCmd { NH_MARK_UP, NH_MARK_DOWN };
-
-struct NHHealthStatus {
- virtual bool isNextHopAvailable(TSHttpTxn txn, const char *hostname, const int port, void *ih = nullptr) = 0;
- virtual void markNextHop(TSHttpTxn txn, const char *hostname, const int port, const NHCmd status, void *ih = nullptr,
- const time_t now = 0) = 0;
- virtual ~NHHealthStatus() {}
-};
-
-struct NHPluginStrategy {
- virtual void findNextHop(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual bool nextHopExists(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual ~NHPluginStrategy() {}
-
- NHHealthStatus *healthStatus;
-};
+// Plugins may set this to indicate how to retry.
+//
+// If handled is false, then no plugin set it, and Core will proceed to do its own thing.
+//
+// If handled is true, core will not do any parent processing, markdown, or anything else,
+// but will use the values in this for whether to use the existing response or make another request,
+// and what that request should look like.
+//
+// See the API functions which take this for ownership requirements of pointers, like hostname.
+//
+// DO NOT put non-POD data in this. It's passed by-copy from API calls.
+//
+typedef struct {
+ // TODO this shouldn't be necessary - plugins should manipulate the response as they see fit,
+ // core shouldn't "know" if it was a "success" or "failure," only the response or retry data/action.
+ // But for now, core needs to know, for reasons.
+ bool fail;
+ const char *hostname;
Review comment:
Gah. That defeats the point of having that API. I guess change it for now and put a note and I'll need to fix the internals at some later point.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r576924046
##########
File path: include/ts/nexthop.h
##########
@@ -28,22 +28,28 @@
#pragma once
-#include <ts/apidefs.h>
-
-// plugin callback commands.
-enum NHCmd { NH_MARK_UP, NH_MARK_DOWN };
-
-struct NHHealthStatus {
- virtual bool isNextHopAvailable(TSHttpTxn txn, const char *hostname, const int port, void *ih = nullptr) = 0;
- virtual void markNextHop(TSHttpTxn txn, const char *hostname, const int port, const NHCmd status, void *ih = nullptr,
- const time_t now = 0) = 0;
- virtual ~NHHealthStatus() {}
-};
-
-struct NHPluginStrategy {
- virtual void findNextHop(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual bool nextHopExists(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual ~NHPluginStrategy() {}
-
- NHHealthStatus *healthStatus;
-};
+// Plugins may set this to indicate how to retry.
+//
+// If handled is false, then no plugin set it, and Core will proceed to do its own thing.
+//
+// If handled is true, core will not do any parent processing, markdown, or anything else,
+// but will use the values in this for whether to use the existing response or make another request,
+// and what that request should look like.
+//
+// See the API functions which take this for ownership requirements of pointers, like hostname.
+//
+// DO NOT put non-POD data in this. It's passed by-copy from API calls.
+//
+typedef struct {
+ // TODO this shouldn't be necessary - plugins should manipulate the response as they see fit,
+ // core shouldn't "know" if it was a "success" or "failure," only the response or retry data/action.
+ // But for now, core needs to know, for reasons.
+ bool fail;
Review comment:
Move this down so all the `bool` values are together. I wonder if they should be changed to bit fields.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r580655423
##########
File path: include/ts/ts.h
##########
@@ -30,6 +30,7 @@
#pragma once
#include <ts/apidefs.h>
+#include <ts/nexthop.h>
Review comment:
"upstream_select.h" - I think "parent" is a poor legacy name. But it's not that important.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ezelkow1 commented on pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
ezelkow1 commented on pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#issuecomment-812070807
[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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r576937125
##########
File path: include/ts/ts.h
##########
@@ -2601,6 +2602,12 @@ tsapi TSReturnCode TSRemapFromUrlGet(TSHttpTxn txnp, TSMLoc *urlLocp);
//
tsapi TSReturnCode TSRemapToUrlGet(TSHttpTxn txnp, TSMLoc *urlLocp);
+// Override response behavior, and hard-set the state machine for whether to succeed or fail, and how.
+tsapi void TSHttpTxnResponseActionSet(TSHttpTxn txnp, TSResponseAction action);
Review comment:
I was on the fence. I went with value, because it needs copied anyway. But yeah, it's going to be 2 copies, that the compiler probably can't optimize. I don't object to changing 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r582076989
##########
File path: include/ts/nexthop.h
##########
@@ -28,22 +28,28 @@
#pragma once
-#include <ts/apidefs.h>
-
-// plugin callback commands.
-enum NHCmd { NH_MARK_UP, NH_MARK_DOWN };
-
-struct NHHealthStatus {
- virtual bool isNextHopAvailable(TSHttpTxn txn, const char *hostname, const int port, void *ih = nullptr) = 0;
- virtual void markNextHop(TSHttpTxn txn, const char *hostname, const int port, const NHCmd status, void *ih = nullptr,
- const time_t now = 0) = 0;
- virtual ~NHHealthStatus() {}
-};
-
-struct NHPluginStrategy {
- virtual void findNextHop(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual bool nextHopExists(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual ~NHPluginStrategy() {}
-
- NHHealthStatus *healthStatus;
-};
+// Plugins may set this to indicate how to retry.
+//
+// If handled is false, then no plugin set it, and Core will proceed to do its own thing.
+//
+// If handled is true, core will not do any parent processing, markdown, or anything else,
+// but will use the values in this for whether to use the existing response or make another request,
+// and what that request should look like.
+//
+// See the API functions which take this for ownership requirements of pointers, like hostname.
+//
+// DO NOT put non-POD data in this. It's passed by-copy from API calls.
+//
+typedef struct {
+ // TODO this shouldn't be necessary - plugins should manipulate the response as they see fit,
+ // core shouldn't "know" if it was a "success" or "failure," only the response or retry data/action.
+ // But for now, core needs to know, for reasons.
+ bool fail;
+ const char *hostname;
Review comment:
Changed to take a length
##########
File path: include/ts/nexthop.h
##########
@@ -28,22 +28,28 @@
#pragma once
-#include <ts/apidefs.h>
-
-// plugin callback commands.
-enum NHCmd { NH_MARK_UP, NH_MARK_DOWN };
-
-struct NHHealthStatus {
- virtual bool isNextHopAvailable(TSHttpTxn txn, const char *hostname, const int port, void *ih = nullptr) = 0;
- virtual void markNextHop(TSHttpTxn txn, const char *hostname, const int port, const NHCmd status, void *ih = nullptr,
- const time_t now = 0) = 0;
- virtual ~NHHealthStatus() {}
-};
-
-struct NHPluginStrategy {
- virtual void findNextHop(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual bool nextHopExists(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual ~NHPluginStrategy() {}
-
- NHHealthStatus *healthStatus;
-};
+// Plugins may set this to indicate how to retry.
+//
+// If handled is false, then no plugin set it, and Core will proceed to do its own thing.
+//
+// If handled is true, core will not do any parent processing, markdown, or anything else,
+// but will use the values in this for whether to use the existing response or make another request,
+// and what that request should look like.
+//
+// See the API functions which take this for ownership requirements of pointers, like hostname.
+//
+// DO NOT put non-POD data in this. It's passed by-copy from API calls.
+//
+typedef struct {
+ // TODO this shouldn't be necessary - plugins should manipulate the response as they see fit,
+ // core shouldn't "know" if it was a "success" or "failure," only the response or retry data/action.
+ // But for now, core needs to know, for reasons.
+ bool fail;
+ const char *hostname;
+ int port;
Review comment:
Fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r576991520
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -178,12 +178,22 @@ inline static void
findParent(HttpTransact::State *s)
{
url_mapping *mp = s->url_map.getMapping();
-
- if (mp && mp->strategy) {
- return mp->strategy->findNextHop(reinterpret_cast<TSHttpTxn>(s->state_machine));
+ if (s->response_action.handled) {
+ s->parent_result.hostname = s->response_action.action.hostname;
+ s->parent_result.port = s->response_action.action.port;
+ s->parent_result.retry = s->response_action.action.retry;
Review comment:
The plugin needs to know, for markdown and other things. The core does know, it's tracked in `parent_result.retry`. I guess we could add code to set that independent of the `response_action`, but that code doesn't exist today, it was previously in the old parent path that's not taken for response_action/strategy. I'm not sure exactly where it'd have to be added.
I'm also not 100% sure there isn't a use case where a plugin would want to set retry=false even though it is, to elicit certain behavior. It seems strange, but I'm not certain it isn't valid.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] jrushford commented on pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#issuecomment-812589577
[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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r607154313
##########
File path: include/ts/ts.h
##########
@@ -30,6 +30,7 @@
#pragma once
#include <ts/apidefs.h>
+#include <ts/nexthop.h>
Review comment:
Meh. We're going to have another round of changes before 10 ships, so I'm fine with waiting until we're more confident about those changes. We could also put it off until 11 as well.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r576923199
##########
File path: include/ts/nexthop.h
##########
@@ -28,22 +28,28 @@
#pragma once
-#include <ts/apidefs.h>
-
-// plugin callback commands.
-enum NHCmd { NH_MARK_UP, NH_MARK_DOWN };
-
-struct NHHealthStatus {
- virtual bool isNextHopAvailable(TSHttpTxn txn, const char *hostname, const int port, void *ih = nullptr) = 0;
- virtual void markNextHop(TSHttpTxn txn, const char *hostname, const int port, const NHCmd status, void *ih = nullptr,
- const time_t now = 0) = 0;
- virtual ~NHHealthStatus() {}
-};
-
-struct NHPluginStrategy {
- virtual void findNextHop(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual bool nextHopExists(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual ~NHPluginStrategy() {}
-
- NHHealthStatus *healthStatus;
-};
+// Plugins may set this to indicate how to retry.
+//
+// If handled is false, then no plugin set it, and Core will proceed to do its own thing.
+//
+// If handled is true, core will not do any parent processing, markdown, or anything else,
+// but will use the values in this for whether to use the existing response or make another request,
+// and what that request should look like.
+//
+// See the API functions which take this for ownership requirements of pointers, like hostname.
+//
+// DO NOT put non-POD data in this. It's passed by-copy from API calls.
+//
+typedef struct {
+ // TODO this shouldn't be necessary - plugins should manipulate the response as they see fit,
+ // core shouldn't "know" if it was a "success" or "failure," only the response or retry data/action.
+ // But for now, core needs to know, for reasons.
+ bool fail;
+ const char *hostname;
Review comment:
Add `length` - don't require a C-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r576925531
##########
File path: include/ts/ts.h
##########
@@ -2601,6 +2602,12 @@ tsapi TSReturnCode TSRemapFromUrlGet(TSHttpTxn txnp, TSMLoc *urlLocp);
//
tsapi TSReturnCode TSRemapToUrlGet(TSHttpTxn txnp, TSMLoc *urlLocp);
+// Override response behavior, and hard-set the state machine for whether to succeed or fail, and how.
+tsapi void TSHttpTxnResponseActionSet(TSHttpTxn txnp, TSResponseAction action);
Review comment:
Pass by value? `TSResponseAction const* action`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] jrushford commented on pull request #7467: WIP Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#issuecomment-772663628
@rob05c you'll need to run 'make clang-format'
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt removed a comment on pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
bneradt removed a comment on pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#issuecomment-779967139
[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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r576924046
##########
File path: include/ts/nexthop.h
##########
@@ -28,22 +28,28 @@
#pragma once
-#include <ts/apidefs.h>
-
-// plugin callback commands.
-enum NHCmd { NH_MARK_UP, NH_MARK_DOWN };
-
-struct NHHealthStatus {
- virtual bool isNextHopAvailable(TSHttpTxn txn, const char *hostname, const int port, void *ih = nullptr) = 0;
- virtual void markNextHop(TSHttpTxn txn, const char *hostname, const int port, const NHCmd status, void *ih = nullptr,
- const time_t now = 0) = 0;
- virtual ~NHHealthStatus() {}
-};
-
-struct NHPluginStrategy {
- virtual void findNextHop(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual bool nextHopExists(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual ~NHPluginStrategy() {}
-
- NHHealthStatus *healthStatus;
-};
+// Plugins may set this to indicate how to retry.
+//
+// If handled is false, then no plugin set it, and Core will proceed to do its own thing.
+//
+// If handled is true, core will not do any parent processing, markdown, or anything else,
+// but will use the values in this for whether to use the existing response or make another request,
+// and what that request should look like.
+//
+// See the API functions which take this for ownership requirements of pointers, like hostname.
+//
+// DO NOT put non-POD data in this. It's passed by-copy from API calls.
+//
+typedef struct {
+ // TODO this shouldn't be necessary - plugins should manipulate the response as they see fit,
+ // core shouldn't "know" if it was a "success" or "failure," only the response or retry data/action.
+ // But for now, core needs to know, for reasons.
+ bool fail;
Review comment:
Move this down so all the `bool` values are together. I wonder if they should be changed to bit fields. Is `bool` even a value C type?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r580655613
##########
File path: include/ts/ts.h
##########
@@ -2601,6 +2602,12 @@ tsapi TSReturnCode TSRemapFromUrlGet(TSHttpTxn txnp, TSMLoc *urlLocp);
//
tsapi TSReturnCode TSRemapToUrlGet(TSHttpTxn txnp, TSMLoc *urlLocp);
+// Override response behavior, and hard-set the state machine for whether to succeed or fail, and how.
+tsapi void TSHttpTxnResponseActionSet(TSHttpTxn txnp, TSResponseAction action);
Review comment:
Just from an API consistency point of view, it should be a pointer.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r580603734
##########
File path: include/ts/nexthop.h
##########
@@ -28,22 +28,28 @@
#pragma once
-#include <ts/apidefs.h>
-
-// plugin callback commands.
-enum NHCmd { NH_MARK_UP, NH_MARK_DOWN };
-
-struct NHHealthStatus {
- virtual bool isNextHopAvailable(TSHttpTxn txn, const char *hostname, const int port, void *ih = nullptr) = 0;
- virtual void markNextHop(TSHttpTxn txn, const char *hostname, const int port, const NHCmd status, void *ih = nullptr,
- const time_t now = 0) = 0;
- virtual ~NHHealthStatus() {}
-};
-
-struct NHPluginStrategy {
- virtual void findNextHop(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual bool nextHopExists(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual ~NHPluginStrategy() {}
-
- NHHealthStatus *healthStatus;
-};
+// Plugins may set this to indicate how to retry.
+//
+// If handled is false, then no plugin set it, and Core will proceed to do its own thing.
+//
+// If handled is true, core will not do any parent processing, markdown, or anything else,
+// but will use the values in this for whether to use the existing response or make another request,
+// and what that request should look like.
+//
+// See the API functions which take this for ownership requirements of pointers, like hostname.
+//
+// DO NOT put non-POD data in this. It's passed by-copy from API calls.
+//
+typedef struct {
+ // TODO this shouldn't be necessary - plugins should manipulate the response as they see fit,
+ // core shouldn't "know" if it was a "success" or "failure," only the response or retry data/action.
+ // But for now, core needs to know, for reasons.
+ bool fail;
+ const char *hostname;
Review comment:
Should I comment/document that the hostname must still be null-terminated?
`std::string_view` isn't necessarily null-terminated, but in core this hostname is set to `s->parent_result.hostname` and all of core is expecting a null-terminated string. Allowing non-null-terminated `respose_action.hostname` would require a lot of work and checking, and would be very difficult to be sure it wasn't missed somewhere.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ezelkow1 commented on pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
ezelkow1 commented on pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#issuecomment-812077486
[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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] jrushford commented on pull request #7467: WIP Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#issuecomment-772757663
@rob05c a change has broken the unit tests in `/home/jrushf1239k/trafficserver/proxy/http/remap`. Try running the unit tests there.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r582079056
##########
File path: include/ts/ts.h
##########
@@ -30,6 +30,7 @@
#pragma once
#include <ts/apidefs.h>
+#include <ts/nexthop.h>
Review comment:
I changed it to `parentselectdefs.h` before I saw this comment.
I don't have a strong opinion on the name, as long as it's consistent. If we name that file `upstream_select.h`, I'd like to rename the whole plugin `upstream_select` to match
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r580326041
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -178,12 +178,22 @@ inline static void
findParent(HttpTransact::State *s)
{
url_mapping *mp = s->url_map.getMapping();
-
- if (mp && mp->strategy) {
- return mp->strategy->findNextHop(reinterpret_cast<TSHttpTxn>(s->state_machine));
+ if (s->response_action.handled) {
+ s->parent_result.hostname = s->response_action.action.hostname;
+ s->parent_result.port = s->response_action.action.port;
+ s->parent_result.retry = s->response_action.action.retry;
Review comment:
Looking at the HostDB restructure and other discussions, the core will need to track the number of retries per upstream and per transaction.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r576942775
##########
File path: include/ts/nexthop.h
##########
@@ -28,22 +28,28 @@
#pragma once
-#include <ts/apidefs.h>
-
-// plugin callback commands.
-enum NHCmd { NH_MARK_UP, NH_MARK_DOWN };
-
-struct NHHealthStatus {
- virtual bool isNextHopAvailable(TSHttpTxn txn, const char *hostname, const int port, void *ih = nullptr) = 0;
- virtual void markNextHop(TSHttpTxn txn, const char *hostname, const int port, const NHCmd status, void *ih = nullptr,
- const time_t now = 0) = 0;
- virtual ~NHHealthStatus() {}
-};
-
-struct NHPluginStrategy {
- virtual void findNextHop(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual bool nextHopExists(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual ~NHPluginStrategy() {}
-
- NHHealthStatus *healthStatus;
-};
+// Plugins may set this to indicate how to retry.
+//
+// If handled is false, then no plugin set it, and Core will proceed to do its own thing.
+//
+// If handled is true, core will not do any parent processing, markdown, or anything else,
+// but will use the values in this for whether to use the existing response or make another request,
+// and what that request should look like.
+//
+// See the API functions which take this for ownership requirements of pointers, like hostname.
+//
+// DO NOT put non-POD data in this. It's passed by-copy from API calls.
+//
+typedef struct {
+ // TODO this shouldn't be necessary - plugins should manipulate the response as they see fit,
+ // core shouldn't "know" if it was a "success" or "failure," only the response or retry data/action.
+ // But for now, core needs to know, for reasons.
+ bool fail;
+ const char *hostname;
+ int port;
+ bool retry; // whether this request is retrying a previously failed parent
Review comment:
Need a better name - `is_retry`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r576989000
##########
File path: include/ts/nexthop.h
##########
@@ -28,22 +28,28 @@
#pragma once
-#include <ts/apidefs.h>
-
-// plugin callback commands.
-enum NHCmd { NH_MARK_UP, NH_MARK_DOWN };
-
-struct NHHealthStatus {
- virtual bool isNextHopAvailable(TSHttpTxn txn, const char *hostname, const int port, void *ih = nullptr) = 0;
- virtual void markNextHop(TSHttpTxn txn, const char *hostname, const int port, const NHCmd status, void *ih = nullptr,
- const time_t now = 0) = 0;
- virtual ~NHHealthStatus() {}
-};
-
-struct NHPluginStrategy {
- virtual void findNextHop(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual bool nextHopExists(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual ~NHPluginStrategy() {}
-
- NHHealthStatus *healthStatus;
-};
+// Plugins may set this to indicate how to retry.
+//
+// If handled is false, then no plugin set it, and Core will proceed to do its own thing.
+//
+// If handled is true, core will not do any parent processing, markdown, or anything else,
+// but will use the values in this for whether to use the existing response or make another request,
+// and what that request should look like.
+//
+// See the API functions which take this for ownership requirements of pointers, like hostname.
+//
+// DO NOT put non-POD data in this. It's passed by-copy from API calls.
+//
+typedef struct {
+ // TODO this shouldn't be necessary - plugins should manipulate the response as they see fit,
+ // core shouldn't "know" if it was a "success" or "failure," only the response or retry data/action.
+ // But for now, core needs to know, for reasons.
+ bool fail;
+ const char *hostname;
+ int port;
+ bool retry; // whether this request is retrying a previously failed parent
Review comment:
Agreed. It was legacy from the core name. I'll fix
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r576925055
##########
File path: include/ts/ts.h
##########
@@ -30,6 +30,7 @@
#pragma once
#include <ts/apidefs.h>
+#include <ts/nexthop.h>
Review comment:
Can you make this "nexthopdefs.h" to be consistent with "apidefs.h"?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c removed a comment on pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
rob05c removed a comment on pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#issuecomment-781443881
> The parent_result->retry is set when the retry window on a marked down parent elapses. In HttpTransact, if retry is true and the transaction is successful to the parent, HttpTransact will mark the parent up. That’s the only way markParentUp is ever called.
Hm, if markParent* is the only thing that's needed for, maybe it can be removed. The markParent* calls aren't done by Core for the Plugin, it does its own markdown entirely self-contained. I'll look over the code, to see if maybe that field can just be 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r580657998
##########
File path: include/ts/nexthop.h
##########
@@ -28,22 +28,28 @@
#pragma once
-#include <ts/apidefs.h>
-
-// plugin callback commands.
-enum NHCmd { NH_MARK_UP, NH_MARK_DOWN };
-
-struct NHHealthStatus {
- virtual bool isNextHopAvailable(TSHttpTxn txn, const char *hostname, const int port, void *ih = nullptr) = 0;
- virtual void markNextHop(TSHttpTxn txn, const char *hostname, const int port, const NHCmd status, void *ih = nullptr,
- const time_t now = 0) = 0;
- virtual ~NHHealthStatus() {}
-};
-
-struct NHPluginStrategy {
- virtual void findNextHop(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual bool nextHopExists(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual ~NHPluginStrategy() {}
-
- NHHealthStatus *healthStatus;
-};
+// Plugins may set this to indicate how to retry.
+//
+// If handled is false, then no plugin set it, and Core will proceed to do its own thing.
+//
+// If handled is true, core will not do any parent processing, markdown, or anything else,
+// but will use the values in this for whether to use the existing response or make another request,
+// and what that request should look like.
+//
+// See the API functions which take this for ownership requirements of pointers, like hostname.
+//
+// DO NOT put non-POD data in this. It's passed by-copy from API calls.
+//
+typedef struct {
+ // TODO this shouldn't be necessary - plugins should manipulate the response as they see fit,
+ // core shouldn't "know" if it was a "success" or "failure," only the response or retry data/action.
+ // But for now, core needs to know, for reasons.
+ bool fail;
+ const char *hostname;
Review comment:
Yeah, that char* is passed around pretty carelessly in core, unfortunately.
Well, if we document it as requiring a null terminator for now, we can remove that requirement later, after we've cleaned up core.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c removed a comment on pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
rob05c removed a comment on pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#issuecomment-781457408
Yeah, I don't see any other uses in Core, except that `markParentUp`. The plugin still needs to know, I'll look into moving that into the plugin's self-contained `strategyTxn`. That'll be good, one less thing in core.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r576923578
##########
File path: include/ts/nexthop.h
##########
@@ -28,22 +28,28 @@
#pragma once
-#include <ts/apidefs.h>
-
-// plugin callback commands.
-enum NHCmd { NH_MARK_UP, NH_MARK_DOWN };
-
-struct NHHealthStatus {
- virtual bool isNextHopAvailable(TSHttpTxn txn, const char *hostname, const int port, void *ih = nullptr) = 0;
- virtual void markNextHop(TSHttpTxn txn, const char *hostname, const int port, const NHCmd status, void *ih = nullptr,
- const time_t now = 0) = 0;
- virtual ~NHHealthStatus() {}
-};
-
-struct NHPluginStrategy {
- virtual void findNextHop(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual bool nextHopExists(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual ~NHPluginStrategy() {}
-
- NHHealthStatus *healthStatus;
-};
+// Plugins may set this to indicate how to retry.
+//
+// If handled is false, then no plugin set it, and Core will proceed to do its own thing.
+//
+// If handled is true, core will not do any parent processing, markdown, or anything else,
+// but will use the values in this for whether to use the existing response or make another request,
+// and what that request should look like.
+//
+// See the API functions which take this for ownership requirements of pointers, like hostname.
+//
+// DO NOT put non-POD data in this. It's passed by-copy from API calls.
+//
+typedef struct {
+ // TODO this shouldn't be necessary - plugins should manipulate the response as they see fit,
+ // core shouldn't "know" if it was a "success" or "failure," only the response or retry data/action.
+ // But for now, core needs to know, for reasons.
+ bool fail;
+ const char *hostname;
+ int port;
Review comment:
`in_port_t port;`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] jrushford commented on pull request #7467: WIP Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#issuecomment-772668097
@rob05c something went wrong, I can't compile as the branch seems to be missing `tscpp/api/parentresult.h`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on a change in pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#discussion_r576939764
##########
File path: include/ts/nexthop.h
##########
@@ -28,22 +28,28 @@
#pragma once
-#include <ts/apidefs.h>
-
-// plugin callback commands.
-enum NHCmd { NH_MARK_UP, NH_MARK_DOWN };
-
-struct NHHealthStatus {
- virtual bool isNextHopAvailable(TSHttpTxn txn, const char *hostname, const int port, void *ih = nullptr) = 0;
- virtual void markNextHop(TSHttpTxn txn, const char *hostname, const int port, const NHCmd status, void *ih = nullptr,
- const time_t now = 0) = 0;
- virtual ~NHHealthStatus() {}
-};
-
-struct NHPluginStrategy {
- virtual void findNextHop(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual bool nextHopExists(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual ~NHPluginStrategy() {}
-
- NHHealthStatus *healthStatus;
-};
+// Plugins may set this to indicate how to retry.
+//
+// If handled is false, then no plugin set it, and Core will proceed to do its own thing.
+//
+// If handled is true, core will not do any parent processing, markdown, or anything else,
+// but will use the values in this for whether to use the existing response or make another request,
+// and what that request should look like.
+//
+// See the API functions which take this for ownership requirements of pointers, like hostname.
+//
+// DO NOT put non-POD data in this. It's passed by-copy from API calls.
+//
+typedef struct {
+ // TODO this shouldn't be necessary - plugins should manipulate the response as they see fit,
+ // core shouldn't "know" if it was a "success" or "failure," only the response or retry data/action.
+ // But for now, core needs to know, for reasons.
+ bool fail;
Review comment:
`bool` is C99. I think I'd prefer to keep bool, I don't think the performance or space matter, and it's more readable and easier to work with.
But I'll definitely move them together, so the compiler will hopefully pack the struct.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] jrushford commented on pull request #7467: Parent Select Plugin
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #7467:
URL: https://github.com/apache/trafficserver/pull/7467#issuecomment-781438393
The parent_result->retry is set when the retry window on a marked down parent elapses. In HttpTransact, if retry is true and the transaction is successful to the parent, HttpTransact will mark the parent up. That’s the only way markParentUp is ever called.
From: Robert O Butts <no...@github.com>
Date: Tuesday, February 16, 2021 at 10:11 AM
To: apache/trafficserver <tr...@noreply.github.com>
Cc: Rushford, John <Jo...@cable.comcast.com>, Mention <me...@noreply.github.com>
Subject: Re: [apache/trafficserver] Parent Select Plugin (#7467)
@rob05c commented on this pull request.
________________________________
In proxy/http/HttpTransact.cc<https://urldefense.com/v3/__https:/github.com/apache/trafficserver/pull/7467*discussion_r576991520__;Iw!!CQl3mcHX2A!UZlne5ZEj-rTs35tBbyWCwBIUiORa1xSvCjhLcH4yG6lTwkpduxv5WYD7WOeZFqpQUq9slA$>:
> @@ -178,12 +178,22 @@ inline static void
findParent(HttpTransact::State *s)
{
url_mapping *mp = s->url_map.getMapping();
-
- if (mp && mp->strategy) {
- return mp->strategy->findNextHop(reinterpret_cast<TSHttpTxn>(s->state_machine));
+ if (s->response_action.handled) {
+ s->parent_result.hostname = s->response_action.action.hostname;
+ s->parent_result.port = s->response_action.action.port;
+ s->parent_result.retry = s->response_action.action.retry;
The plugin needs to know, for markdown and other things. The core does know, it's tracked in parent_result.retry. I guess we could add code to set that independent of the response_action, but that code doesn't exist today, it was previously in the old parent path that's not taken for response_action/strategy. I'm not sure exactly where it'd have to be added.
I'm also not 100% sure there isn't a use case where a plugin would want to set retry=false even though it is, to elicit certain behavior. It seems strange, but I'm not certain it isn't valid.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https:/github.com/apache/trafficserver/pull/7467*discussion_r576991520__;Iw!!CQl3mcHX2A!UZlne5ZEj-rTs35tBbyWCwBIUiORa1xSvCjhLcH4yG6lTwkpduxv5WYD7WOeZFqpQUq9slA$>, or unsubscribe<https://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/ABCXE3TCFEWAE2NSBQSFM4TS7KRLPANCNFSM4W5RGSFQ__;!!CQl3mcHX2A!UZlne5ZEj-rTs35tBbyWCwBIUiORa1xSvCjhLcH4yG6lTwkpduxv5WYD7WOeZFqp0NewP1o$>.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org