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/07/19 21:31:57 UTC
[GitHub] [trafficserver] elsloo opened a new pull request #8088: Allow variable buffer sizes and watermarks with transaction intercept plugins
elsloo opened a new pull request #8088:
URL: https://github.com/apache/trafficserver/pull/8088
* Allow plugins and users of the API to specify buffer size indexes and watermarks
* Adds two new overridable `records.config` parameters to control the PluginVC interface specifically to allow for independent tuning of these settings and the other default buffer size index and watermark parameters
* Resolves #8087
--
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] elsloo commented on a change in pull request #8088: Allow variable buffer sizes and watermarks with transaction intercept plugins
Posted by GitBox <gi...@apache.org>.
elsloo commented on a change in pull request #8088:
URL: https://github.com/apache/trafficserver/pull/8088#discussion_r678625518
##########
File path: doc/developer-guide/api/functions/TSHttpConnectPlugin.en.rst
##########
@@ -0,0 +1,89 @@
+.. Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed
+ with this work for additional information regarding copyright
+ ownership. The ASF licenses this file to you under the Apache
+ License, Version 2.0 (the "License"); you may not use this file
+ except in compliance with the License. You may obtain a copy of
+ the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ implied. See the License for the specific language governing
+ permissions and limitations under the License.
+
+.. include:: ../../../common.defs
+
+.. default-domain:: c
+
+TSHttpConnectPlugin
+*************************
+
+Allows the plugin to initiate an http connection. This will tag the
+HTTP state machine with extra data that can be accessed by the
+logging interface. Additional arguments provide buffer settings that
+are used when constructing IOBuffers. The connection is treated as
+an HTTP transaction as if it came from a client.
+
+Synopsis
+========
+
+.. code-block:: cpp
+
+ #include <ts/ts.h>
+
+.. function:: TSVConn TSHttpConnectPlugin(TSHttpConnectPluginOptions options);
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] traeak commented on a change in pull request #8088: Allow variable buffer sizes and watermarks with transaction intercept plugins
Posted by GitBox <gi...@apache.org>.
traeak commented on a change in pull request #8088:
URL: https://github.com/apache/trafficserver/pull/8088#discussion_r677010274
##########
File path: include/ts/ts.h
##########
@@ -1778,6 +1778,14 @@ tsapi void TSHttpTxnServerIntercept(TSCont contp, TSHttpTxn txnp);
@param addr Target address of the origin server.
@param tag A logging tag that can be accessed via the pitag field. May be @c NULL.
@param id A logging id that can be access via the piid field.
+ @param buffer_index Index for read and write buffers used by PluginVC.
+ @param buffer_water_mark Number of bytes used as the water mark for buffers used by PluginVC.
+ */
+tsapi TSVConn TSHttpConnectPlugin(struct sockaddr const *addr, const char *tag, int64_t id, int64_t buffer_index,
+ int64_t buffer_water_mark);
Review comment:
Should that buffer_index value have enumeration/int64_t values defined in apidefs.h that parallel these values in core? There are 15 total values:
BUFFER_SIZE_INDEX_128
...
BUFFER_SIZE_INDEX_32K
...
--
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] bryancall commented on pull request #8088: Allow variable buffer sizes and watermarks with transaction intercept plugins
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #8088:
URL: https://github.com/apache/trafficserver/pull/8088#issuecomment-882918628
@ywkaras volunteered to look at 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] elsloo commented on a change in pull request #8088: Allow variable buffer sizes and watermarks with transaction intercept plugins
Posted by GitBox <gi...@apache.org>.
elsloo commented on a change in pull request #8088:
URL: https://github.com/apache/trafficserver/pull/8088#discussion_r679374725
##########
File path: doc/developer-guide/api/functions/TSTypes.en.rst
##########
@@ -78,6 +78,34 @@ more widely. Those are described on this page.
A 64 bit time value, measured in nanoseconds.
+.. type:: TSHttpConnectPluginOptions
Review comment:
@SolidWallOfCode Please take a look at the `TSHttpConnectOptionsGet(TSConnectType connect_type)` API function and its uses.
--
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] elsloo commented on a change in pull request #8088: Allow variable buffer sizes and watermarks with transaction intercept plugins
Posted by GitBox <gi...@apache.org>.
elsloo commented on a change in pull request #8088:
URL: https://github.com/apache/trafficserver/pull/8088#discussion_r675756102
##########
File path: include/ts/ts.h
##########
@@ -1778,6 +1778,14 @@ tsapi void TSHttpTxnServerIntercept(TSCont contp, TSHttpTxn txnp);
@param addr Target address of the origin server.
@param tag A logging tag that can be accessed via the pitag field. May be @c NULL.
@param id A logging id that can be access via the piid field.
+ @param buffer_index Index for read and write buffers used by PluginVC.
+ @param buffer_water_mark Number of bytes used as the water mark for buffers used by PluginVC.
+ */
+tsapi TSVConn TSHttpConnectPlugin(struct sockaddr const *addr, const char *tag, int64_t id, int64_t buffer_index,
+ int64_t buffer_water_mark);
Review comment:
I just pushed a commit to address this; please take a look at your convenience.
--
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] elsloo commented on a change in pull request #8088: Allow variable buffer sizes and watermarks with transaction intercept plugins
Posted by GitBox <gi...@apache.org>.
elsloo commented on a change in pull request #8088:
URL: https://github.com/apache/trafficserver/pull/8088#discussion_r675763105
##########
File path: src/traffic_server/InkAPI.cc
##########
@@ -6880,14 +6880,28 @@ TSHttpTxnPluginTagGet(TSHttpTxn txnp)
TSVConn
TSHttpConnectWithPluginId(sockaddr const *addr, const char *tag, int64_t id)
+{
+ return TSHttpConnectPlugin(addr, tag, id, BUFFER_SIZE_INDEX_32K, DEFAULT_PLUGIN_VC_BUFFER_WATER_MARK);
+}
+
+TSVConn
+TSHttpConnectPlugin(sockaddr const *addr, const char *tag, int64_t id, int64_t buffer_index, int64_t buffer_water_mark)
{
sdk_assert(addr);
sdk_assert(ats_is_ip(addr));
sdk_assert(ats_ip_port_cast(addr));
+ if (buffer_index < BUFFER_SIZE_INDEX_128 || buffer_index > MAX_BUFFER_SIZE_INDEX) {
+ buffer_index = BUFFER_SIZE_INDEX_32K; // out of range, set to the default for safety
+ }
+
+ if (buffer_water_mark < 0) {
+ buffer_water_mark = DEFAULT_PLUGIN_VC_BUFFER_WATER_MARK;
+ }
Review comment:
I believe this is the correct behavior because there is no assert on these inputs. I originally had asserts in place of the if statements with similar logic but removed them due to the risk of crashing ATS if/when a new overridden parameter comes in. Ignoring invalid parameters and avoiding a crash seemed like the lesser of two evils, but I'm open to alternate suggestions. This is not the only place where I do this sort of check, and I would prefer to keep the behavior consistent, so if we change it here, I'll want to change it elsewhere in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #8088: Allow variable buffer sizes and watermarks with transaction intercept plugins
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #8088:
URL: https://github.com/apache/trafficserver/pull/8088#discussion_r674216837
##########
File path: include/ts/ts.h
##########
@@ -1778,6 +1778,14 @@ tsapi void TSHttpTxnServerIntercept(TSCont contp, TSHttpTxn txnp);
@param addr Target address of the origin server.
@param tag A logging tag that can be accessed via the pitag field. May be @c NULL.
@param id A logging id that can be access via the piid field.
+ @param buffer_index Index for read and write buffers used by PluginVC.
+ @param buffer_water_mark Number of bytes used as the water mark for buffers used by PluginVC.
+ */
+tsapi TSVConn TSHttpConnectPlugin(struct sockaddr const *addr, const char *tag, int64_t id, int64_t buffer_index,
+ int64_t buffer_water_mark);
Review comment:
Why isn't this documented in the TS manual?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #8088: Allow variable buffer sizes and watermarks with transaction intercept plugins
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #8088:
URL: https://github.com/apache/trafficserver/pull/8088#discussion_r678378048
##########
File path: doc/developer-guide/api/functions/TSTypes.en.rst
##########
@@ -78,6 +78,34 @@ more widely. Those are described on this page.
A 64 bit time value, measured in nanoseconds.
+.. type:: TSHttpConnectPluginOptions
Review comment:
This needs a leading value that indicates what data is valid in it for backwards compatibility. This could be a size, or a version, or both. This must always be first (similar to `sa_family` in `sockaddr`). Alternatively this length could be passed as an argument to `TSHttpConnectPlugin`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #8088: Allow variable buffer sizes and watermarks with transaction intercept plugins
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #8088:
URL: https://github.com/apache/trafficserver/pull/8088#discussion_r678382284
##########
File path: doc/developer-guide/api/functions/TSTypes.en.rst
##########
@@ -78,6 +78,34 @@ more widely. Those are described on this page.
A 64 bit time value, measured in nanoseconds.
+.. type:: TSHttpConnectPluginOptions
Review comment:
A way to mark values as "default" or "not set" is needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #8088: Allow variable buffer sizes and watermarks with transaction intercept plugins
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #8088:
URL: https://github.com/apache/trafficserver/pull/8088#discussion_r678378350
##########
File path: doc/developer-guide/api/functions/TSHttpConnectPlugin.en.rst
##########
@@ -0,0 +1,89 @@
+.. Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed
+ with this work for additional information regarding copyright
+ ownership. The ASF licenses this file to you under the Apache
+ License, Version 2.0 (the "License"); you may not use this file
+ except in compliance with the License. You may obtain a copy of
+ the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ implied. See the License for the specific language governing
+ permissions and limitations under the License.
+
+.. include:: ../../../common.defs
+
+.. default-domain:: c
+
+TSHttpConnectPlugin
+*************************
+
+Allows the plugin to initiate an http connection. This will tag the
+HTTP state machine with extra data that can be accessed by the
+logging interface. Additional arguments provide buffer settings that
+are used when constructing IOBuffers. The connection is treated as
+an HTTP transaction as if it came from a client.
+
+Synopsis
+========
+
+.. code-block:: cpp
+
+ #include <ts/ts.h>
+
+.. function:: TSVConn TSHttpConnectPlugin(TSHttpConnectPluginOptions options);
Review comment:
Pass by 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.
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 #8088: Allow variable buffer sizes and watermarks with transaction intercept plugins
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #8088:
URL: https://github.com/apache/trafficserver/pull/8088#discussion_r674223236
##########
File path: src/traffic_server/InkAPI.cc
##########
@@ -6880,14 +6880,28 @@ TSHttpTxnPluginTagGet(TSHttpTxn txnp)
TSVConn
TSHttpConnectWithPluginId(sockaddr const *addr, const char *tag, int64_t id)
+{
+ return TSHttpConnectPlugin(addr, tag, id, BUFFER_SIZE_INDEX_32K, DEFAULT_PLUGIN_VC_BUFFER_WATER_MARK);
+}
+
+TSVConn
+TSHttpConnectPlugin(sockaddr const *addr, const char *tag, int64_t id, int64_t buffer_index, int64_t buffer_water_mark)
{
sdk_assert(addr);
sdk_assert(ats_is_ip(addr));
sdk_assert(ats_ip_port_cast(addr));
+ if (buffer_index < BUFFER_SIZE_INDEX_128 || buffer_index > MAX_BUFFER_SIZE_INDEX) {
+ buffer_index = BUFFER_SIZE_INDEX_32K; // out of range, set to the default for safety
+ }
+
+ if (buffer_water_mark < 0) {
+ buffer_water_mark = DEFAULT_PLUGIN_VC_BUFFER_WATER_MARK;
+ }
Review comment:
If either of the last two parameters is -1, should it default to the configured 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] bryancall commented on pull request #8088: Allow variable buffer sizes and watermarks with transaction intercept plugins
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #8088:
URL: https://github.com/apache/trafficserver/pull/8088#issuecomment-882918628
@ywkaras volunteered to look at 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 a change in pull request #8088: Allow variable buffer sizes and watermarks with transaction intercept plugins
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #8088:
URL: https://github.com/apache/trafficserver/pull/8088#discussion_r676751975
##########
File path: src/traffic_server/InkAPI.cc
##########
@@ -6880,14 +6880,28 @@ TSHttpTxnPluginTagGet(TSHttpTxn txnp)
TSVConn
TSHttpConnectWithPluginId(sockaddr const *addr, const char *tag, int64_t id)
+{
+ return TSHttpConnectPlugin(addr, tag, id, BUFFER_SIZE_INDEX_32K, DEFAULT_PLUGIN_VC_BUFFER_WATER_MARK);
+}
+
+TSVConn
+TSHttpConnectPlugin(sockaddr const *addr, const char *tag, int64_t id, int64_t buffer_index, int64_t buffer_water_mark)
{
sdk_assert(addr);
sdk_assert(ats_is_ip(addr));
sdk_assert(ats_ip_port_cast(addr));
+ if (buffer_index < BUFFER_SIZE_INDEX_128 || buffer_index > MAX_BUFFER_SIZE_INDEX) {
+ buffer_index = BUFFER_SIZE_INDEX_32K; // out of range, set to the default for safety
+ }
+
+ if (buffer_water_mark < 0) {
+ buffer_water_mark = DEFAULT_PLUGIN_VC_BUFFER_WATER_MARK;
+ }
Review comment:
ok
--
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] vmamidi merged pull request #8088: Allow variable buffer sizes and watermarks with transaction intercept plugins
Posted by GitBox <gi...@apache.org>.
vmamidi merged pull request #8088:
URL: https://github.com/apache/trafficserver/pull/8088
--
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] vmamidi merged pull request #8088: Allow variable buffer sizes and watermarks with transaction intercept plugins
Posted by GitBox <gi...@apache.org>.
vmamidi merged pull request #8088:
URL: https://github.com/apache/trafficserver/pull/8088
--
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] elsloo commented on a change in pull request #8088: Allow variable buffer sizes and watermarks with transaction intercept plugins
Posted by GitBox <gi...@apache.org>.
elsloo commented on a change in pull request #8088:
URL: https://github.com/apache/trafficserver/pull/8088#discussion_r677786941
##########
File path: include/ts/ts.h
##########
@@ -1778,6 +1778,14 @@ tsapi void TSHttpTxnServerIntercept(TSCont contp, TSHttpTxn txnp);
@param addr Target address of the origin server.
@param tag A logging tag that can be accessed via the pitag field. May be @c NULL.
@param id A logging id that can be access via the piid field.
+ @param buffer_index Index for read and write buffers used by PluginVC.
+ @param buffer_water_mark Number of bytes used as the water mark for buffers used by PluginVC.
+ */
+tsapi TSVConn TSHttpConnectPlugin(struct sockaddr const *addr, const char *tag, int64_t id, int64_t buffer_index,
+ int64_t buffer_water_mark);
Review comment:
@traeak I just pushed a commit that introduces a struct (`TSHttpConnectPluginOptions`) to replace the function arguments, and the `buffer_index` member now matches the type you suggest. That said, I am only using that type in the API and its dependencies, and will update PR #8089 to do the same if these changes are accepted.
The changes I made to the PluginVC interface that exist inside the core will continue to use the `#defined` values; this seems to be in the spirit of how these two synonymous things are used in practice (API vs `#define`ed). Do you agree?
--
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] bryancall commented on pull request #8088: Allow variable buffer sizes and watermarks with transaction intercept plugins
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #8088:
URL: https://github.com/apache/trafficserver/pull/8088#issuecomment-882918628
@ywkaras volunteered to look at 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] elsloo commented on a change in pull request #8088: Allow variable buffer sizes and watermarks with transaction intercept plugins
Posted by GitBox <gi...@apache.org>.
elsloo commented on a change in pull request #8088:
URL: https://github.com/apache/trafficserver/pull/8088#discussion_r678625963
##########
File path: doc/developer-guide/api/functions/TSTypes.en.rst
##########
@@ -78,6 +78,34 @@ more widely. Those are described on this page.
A 64 bit time value, measured in nanoseconds.
+.. type:: TSHttpConnectPluginOptions
Review comment:
I _think_ I have captured what you've asked for here in the last commit I just pushed. Please take a look.
--
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