cmcfarlen commented on PR #13616: URL: https://github.com/apache/trafficserver/pull/13616#issuecomment-5515149699
This one is a real bug, and mine. Fixed in 7b19905047. `find()` returned an iterator whose `_it` was the stored id, type bits and all, while `_bound` comes from `current_id()` and is built with COUNTER type bits. For a GAUGE the type bit at `METRIC_TYPE_BITS` puts `_it` above any realistic bound, so `at_end()` was immediately true and the iterator compared equal to `end()`. `find()` was therefore broken for every gauge in the store, not just unlisted ones. `advance()` already normalized the position; the positional constructor did not. It is a regression from the bound comparison I introduced with unlisting — before that, `operator==` compared raw ids and the type bits cancelled out. No production code calls `find()`, so nothing was broken in the field, but every test I wrote for it happened to use a counter, which is why it got through. Fixed as suggested, by keeping only the blob and offset: ```cpp auto [blob, offset] = _metrics._splitID(pos); _it = _makeId(blob, offset, MetricType::COUNTER); ``` Dereferencing is unaffected: `Storage::lookup(id, ...)` deliberately reads the type from the slot rather than the id, precisely because iterators manufacture positions. Test added first and watched fail on `REQUIRE(g != m.end())`, now covering both a gauge and a counter through `find()`, including that the dereferenced type comes back as `GAUGE`. Separately, I have dropped the commit I added earlier about `FlagStorage` initialization. That comment was incorrect and a reply should have been the whole response; the extra commit was noise. -- 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]
