bneradt commented on code in PR #14002:
URL: https://github.com/apache/trafficserver/pull/14002#discussion_r4483163386
##########
tests/uranium_tests/pluginTest/stats_over_http/test_stats_over_http_remap_intercept.py:
##########
@@ -64,6 +64,10 @@ def test_stats_over_http_remap_intercept(ats_factory:
ATSFactory, services: Serv
"proxy.config.diags.debug.enabled": 1,
"proxy.config.diags.debug.tags": "stats_over_http",
"proxy.config.dump_mem_info_frequency": 1,
+ # Linux lets a loopback socket buffer a few megabytes, more than
the whole prometheus body, so without a
+ # small send buffer the intercept can finish its write before the
client's reset arrives and never log the
+ # EOS or ERROR that the test looks for. On macOS the buffer stays
well under the body size either way.
Review Comment:
Nit on the comment: the key *why* is that setting `SO_SNDBUF` explicitly
fixes the buffer size and disables Linux's autotuning (the kernel doubles it,
so this is ~32 KB effective). Saying that would make it clearer why 16 KB is
enough versus the ~1.6 MB body. The macOS sentence could probably go; it
describes a platform where the setting is a no-op for this test rather than why
the setting is here.
##########
tests/uranium_tests/pluginTest/stats_over_http/test_stats_over_http_compression.py:
##########
@@ -38,7 +38,11 @@ def test_stats_over_http_compression(ats_factory:
ATSFactory, services: ServiceF
{
"proxy.config.diags.debug.enabled": 1,
"proxy.config.diags.debug.tags": "test_metrics|stats_over_http",
- "proxy.config.dump_mem_info_frequency": 1
+ "proxy.config.dump_mem_info_frequency": 1,
+ # Linux lets a loopback socket buffer a few megabytes, more than
the whole prometheus body, so without a
+ # small send buffer the intercept can finish its write before the
client's reset arrives and never log the
+ # EOS or ERROR that the test looks for. On macOS the buffer stays
well under the body size either way.
+ "proxy.config.net.sock_send_buffer_size_in": 16384,
Review Comment:
This record and its three-line comment are duplicated verbatim in
`test_stats_over_http_remap_intercept.py`, and `fetch_stats.py` now relies on
every `--reset` caller remembering to set it. Consider defining it once (for
example a `RESET_RECORDS = {"proxy.config.net.sock_send_buffer_size_in":
16384}` next to the client, or in a small shared module/conftest) and
`update()`-ing it in both tests. That keeps the explanation in one place and
makes it hard for a future `--reset` caller to reintroduce the race.
##########
tests/uranium_tests/pluginTest/stats_over_http/fetch_stats.py:
##########
@@ -125,7 +125,8 @@ def fetch_records(args: argparse.Namespace) -> int:
def reset_after_first_byte(args: argparse.Namespace) -> int:
sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
- # A small receive window keeps most of the body in Traffic Server when the
client goes away.
+ # A small receive window, with the small
proxy.config.net.sock_send_buffer_size_in that the --reset callers
+ # set, keeps most of the body in the intercept when the client goes away.
Review Comment:
This comment now documents configuration owned by the callers. If the
setting is centralized as suggested on the compression test, this could just
reference that constant (or the `--reset` help text could mention the
requirement) so the coupling is explicit rather than only described in a
comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
