Copilot commented on code in PR #13616:
URL: https://github.com/apache/trafficserver/pull/13616#discussion_r3916152925
##########
include/tsutil/Metrics.h:
##########
@@ -153,6 +165,36 @@ class Metrics
return _storage->rename(id, name);
}
+ /** Mark @a id as not enumerated, or clear that mark.
+ *
+ * A tombstoned metric keeps its slot, its name and its atomic. It is
skipped by iteration, so it
+ * vanishes from everything that enumerates the store, but it still resolves
through @c lookup and
+ * its value may still be read and written. Creating the same name again
clears the mark and
+ * returns the same id.
+ *
+ * @return @c false if @a id does not name an allocated slot.
+ */
+ bool
+ tombstone(IdType id, bool set = true)
+ {
+ return _storage->tombstone(id, set);
+ }
+
+ bool
+ tombstoned(IdType id) const
+ {
+ return _storage->tombstoned(id);
+ }
+
Review Comment:
The PR description and examples describe a `tombstone()` API (and term), but
the implementation introduces `unlist()`/`relist()` and `UNLISTED`. Please
align the PR description (and any external-facing communication) with the
actual API naming, or provide a compatibility alias (e.g., `tombstone()`
delegating to `unlist()`/`relist()`) if `tombstone` is the intended public term.
##########
src/tsutil/Metrics.cc:
##########
@@ -73,6 +73,14 @@ Metrics::Storage::create(std::string_view name, const
MetricType type)
auto it = _lookups.find(name);
if (it != _lookups.end()) {
+ // Re-creating a name is how a tombstoned metric is resurrected: same
slot, same atomic, and
+ // whatever value it accumulated while it was hidden.
+ auto [blob_ix, offset] = _splitID(it->second);
+
+ if (Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); blob !=
nullptr) {
+ std::get<2>(*blob)[offset].fetch_and(static_cast<uint8_t>(~TOMBSTONE),
MEMORY_ORDER);
+ }
Review Comment:
The bit-twiddling logic to clear/set `UNLISTED` (including the
`static_cast<uint8_t>(~UNLISTED)` mask) is duplicated. Consider centralizing
this into a small helper (e.g., `set_flag(offset, UNLISTED, enabled)` or a
precomputed `constexpr uint8_t CLEAR_UNLISTED_MASK`) to reduce repetition and
make future flag additions less error-prone. (Optional.)
##########
src/tsutil/Metrics.cc:
##########
@@ -245,9 +253,57 @@ Metrics::Storage::rename(Metrics::IdType id,
std::string_view name)
return true;
}
+bool
+Metrics::Storage::tombstone(Metrics::IdType id, bool set)
+{
+ if (!allocated(id)) {
+ return false;
+ }
+
+ auto [blob_ix, offset] = _splitID(id);
+ Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get();
+
+ // Only this bit, so a flag added later is not clobbered by a tombstone or a
resurrect.
+ if (set) {
+ std::get<2>(*blob)[offset].fetch_or(TOMBSTONE, MEMORY_ORDER);
+ } else {
+ std::get<2>(*blob)[offset].fetch_and(static_cast<uint8_t>(~TOMBSTONE),
MEMORY_ORDER);
+ }
Review Comment:
The bit-twiddling logic to clear/set `UNLISTED` (including the
`static_cast<uint8_t>(~UNLISTED)` mask) is duplicated. Consider centralizing
this into a small helper (e.g., `set_flag(offset, UNLISTED, enabled)` or a
precomputed `constexpr uint8_t CLEAR_UNLISTED_MASK`) to reduce repetition and
make future flag additions less error-prone. (Optional.)
--
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]
