Hi Alex,
On Thu, Feb 24, 2022 at 03:03:59AM +0100, Aleksandar Lazic wrote:
> Hi.
>
> Here the first patch for feature request "New Balancing algorithm (Peak) EWMA
> #1570"
Note, I don't think it is needed for this algo as long as we instead
use measured response time and/or health check time. But regardless
it's something useful to have. A few comments below:
> From e95bf6a4bf107fdc59696c4b4a4ef7b03133b813 Mon Sep 17 00:00:00 2001
> From: Aleksandar Lazic <al-hapr...@none.at>
> Date: Thu, 24 Feb 2022 02:56:21 +0100
> Subject: [PATCH] MINOR: sample: Add srv_rtt server round trip time sample
>
> This sample fetch get the server round trip time
You should mention "TCP round trip time" since it's measured at the TCP
level.
> +srv_rtt : integer
> + Returns the Round Trip Time (RTT) measured by the kernel for the server
> + connection. <unit> is facultative, by default the unit is milliseconds.
> <unit>
> + can be set to "ms" for milliseconds or "us" for microseconds. If the server
> + connection is not established, if the connection is not TCP or if the
> + operating system does not support TCP_INFO, for example Linux kernels
> before
> + 2.4, the sample fetch fails.
I would rather call it "bc_rtt" since it's not the server but the backend
connection. Technically speaking it indeed requires a connection to be
established and will only report the value for *this* connection and not
anything stateless related to the server. Thta's more in line with what we
have for the frontend connection already with fc_rtt.
You mentioned "unit" but it does not appear in the keyword syntax.
Also I think it would be useful to have all other fc_* from the same
section turned to bc_* (fc_rttvar, fc_retrans, etc), as it can sometimes
explain long response times in logs.
> diff --git a/reg-tests/sample_fetches/srv_rtt.vtc
> b/reg-tests/sample_fetches/srv_rtt.vtc
> new file mode 100644
> index 000000000..c0ad0cbae
> --- /dev/null
> +++ b/reg-tests/sample_fetches/srv_rtt.vtc
> @@ -0,0 +1,34 @@
> +varnishtest "srv_rtt sample fetch Test"
> +
> +#REQUIRE_VERSION=2.6
Note, we *might* need to add a new macro to detect support for TCP_INFO.
We still don't have the config predicates to detect support for certain
keywords or sample fetch functions so that's not easy, but it's possible
that this test will break on some OS like cygwin. If so we could work
around this temporarily using "EXCLUDE_TARGETS" and in the worst case we
could mark it broken for the time it takes to completely solve this.
(...)
> diff --git a/src/tcp_sample.c b/src/tcp_sample.c
> index 19edcd243..7b8b616cb 100644
> --- a/src/tcp_sample.c
> +++ b/src/tcp_sample.c
> @@ -446,6 +446,20 @@ smp_fetch_fc_reordering(const struct arg *args, struct
> sample *smp, const char *
> return 0;
> return 1;
> }
> +
> +/* get the mean rtt of a client connection */
> +static int
> +smp_fetch_srv_rtt(const struct arg *args, struct sample *smp, const char
> *kw, void *private)
> +{
> + if (!get_tcp_info(args, smp, 1, 0))
> + return 0;
> +
> + /* By default or if explicitly specified, convert rtt to ms */
> + if (!args || args[0].type == ARGT_STOP || args[0].data.sint ==
> TIME_UNIT_MS)
> + smp->data.u.sint = (smp->data.u.sint + 500) / 1000;
> +
> + return 1;
> +}
That's another reason for extending the existing keywords, avoiding code
duplication. You can have all your new keywords map to the fc_* equivalent
and just change this:
- if (!get_tcp_info(args, smp, 0, 0))
+ if (!get_tcp_info(args, smp, *kw == 'b', 0))
Please update the comments on top of the functions to mention that they're
called for both "fc_*" and "bc_*" depending on the side, and that's OK.
thanks,
Willy
