cmcfarlen commented on PR #13666:
URL: https://github.com/apache/trafficserver/pull/13666#issuecomment-5706817283
Three comments, all correct. Two were mechanical; the third found a real
defect and it is worse than described.
**Duplicate test source** — `2c37e2a5aa`. The rebase re-added
`test_ConnectionTracker.cc` to `test_net` after master had already listed it
when master added its own cases to that file.
**Stale assertion message** — `366d7f0d6f`. It named `AGGREGATE_ONLY`,
removed by this PR, so a failure pointed at a configuration that no longer
exists. It now interpolates the configured value.
**Unlisting a name another group owns** — `61d9751174` and `2af1eee7a1`
Confirmed, and the mixed match type case is not the only route. The same
defect reaches through a configuration that overriding `metric_aggregate` is
*for*:
```
mapping A -> multi.origin.com:443 metric_aggregate 3 (sums published)
mapping B -> multi.origin.com:8443 metric_aggregate 2 (max only)
```
Two `MATCH_BOTH` groups, one hostname, one shared set of aggregate names.
Building group B unlisted the aggregate group A was publishing. I checked it
rather than reasoned about it: `is_published("...current_connection.<fqdn>")`
came back false after B was constructed. Same match type throughout.
So the underlying rule is about ownership rather than about match types. A
per group name has exactly one owner and unlisting it is sound. The sums and
the max are named per hostname and shared by every group of that hostname, so
no single group may unlist them.
`ts::Metrics::Derived::remove_source` is the counterpart to `add_source`: it
drops one source and unlists the derived name only when the last source goes,
and adding a source again relists it. Groups that do not publish a shared name
now stop contributing to it instead of unlisting it. The per group names go the
same way even though they have a single source, which has a small side benefit:
the derived pass no longer recomputes a value into a name that is not published.
Both directions of `3 <-> 2` still retract and republish the sums as before,
so nothing was given up to fix this.
Verified the new test fails against the old code: restoring the `unlist`
call on the shared names fails exactly one assertion, the cross-mapping one.
**Documentation was describing the defect**
`records.yaml.en.rst` said the last group rebuilt decides whether the sums
are published, and the enum comment said the same. That was me writing down a
bug as a rule. Both now state that shared names are only withdrawn once no
group of the hostname publishes them, so disagreeing mappings cannot hide each
other's aggregate. `Metrics.en.rst` documents `remove_source` alongside
`add_source`.
`test_tsutil` 16323, `test_records` 335, `[ConnectionTracker]` 87, and the
three `per_server` autests pass in a Fedora 44 container.
--
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]
