mjsax commented on PR #23180: URL: https://github.com/apache/kafka/pull/23180#issuecomment-5350046328
Thanks for the update. I let Claude re-review, and it spit out the following. I did not read the updated PR myself yet, so no idea if Claude is right or wrong: ``` Non-blocking: the reverse-lookup is silently coupled to today's call sites The correctness above rests on two invariants that are true now but unenforced: (a) individual removal only ever happens at client/thread/topic level, and (b) those suffixes never contain .s.. If a future contributor adds an individual removeSensor(Sensor) at node/task/cache level, or introduces a sensor suffix containing .s., the name will silently fail to be removed — reintroducing this exact leak with no test failing to catch it. Cheap ways to de-risk, in rough order of preference: - Have removeSensorName try the synchronized level maps generically (thread → task → node → topic → cache) by the parsed prefix, rather than hardcoding thread+topic. A parsed prefix maps to exactly one level (the prefixes are structurally distinct), so trying them in sequence is safe and cheap, and future individual-removal callers at task/node/cache "just work." Keep store-level excluded — it's bulk-only and lock-free by design, so a synchronized scan there would race the thread-confined creation path. That exclusion is worth a one-line comment. - At minimum, document the two invariants where removeSensorName is defined, so the coupling is explicit. This is a real design point but genuinely optional — the code is correct as submitted. ``` Btw, I noticed something unrelated, which we could cleanup on-the-side in this PR. `parentSensors` is unused -- we should remove it. -- 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]
