SegmentStatusChecker batch read the zk segment metadata instead of per segment - #19252
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19252 +/- ##
============================================
+ Coverage 65.70% 67.11% +1.40%
- Complexity 1423 1424 +1
============================================
Files 3439 3459 +20
Lines 218064 219729 +1665
Branches 34679 34993 +314
============================================
+ Hits 143289 147476 +4187
+ Misses 63226 60475 -2751
- Partials 11549 11778 +229
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| for (String segmentName : segmentNames) { | ||
| paths.add(constructPropertyStorePathForSegment(tableNameWithType, segmentName)); | ||
| } | ||
| List<ZNRecord> znRecords = propertyStore.get(paths, stats, AccessOption.PERSISTENT, false); |
There was a problem hiding this comment.
Why are we passing false for the throwException parameter? Won't passing true instead give exactly the semantics the checker wants: deleted segment → null, failed read → exception, and processTable already has a catch for the latter?
There was a problem hiding this comment.
My reasoning was that one read failure would skip the metric report for the entire table, which becomes a new behavior introduced here. Originally the zk metadata is also null when read failed.
There was a problem hiding this comment.
Thanks for looking. I read the Helix code, and I do not think master returns null on a read failure.
The single-path read on master ends at ZkBaseDataAccessor.get(String, Stat, int):
try {
data = (T) _zkClient.readData(path, stat);
} catch (ZkNoNodeException e) {
if (AccessOption.isThrowExceptionIfNotExist(options)) {
throw e;
}
}Helix catches only ZkNoNodeException. Everything else throws: a connection loss, a timeout, a session expiry, or a deserialization error. processTable catches the exception and calls removeMetricsForTable. The table then loses its gauges.
So a single failed read already skips the whole table on master. It also deletes the gauges, which is more aggressive than the skip here, right?
throwException=true keeps that split. A missing znode still returns null, because Helix never throws on NONODE even with the flag on. Only real failures throw. It is also what getChildren passes on the line you linked in the other thread.
The case that gets worse with false is a partial failure. If 3k reads in a 10k batch fail, those segments look deleted. The checker skips them in the replica check and does not count their sizes in TABLE_COMPRESSED_SIZE. The numSegmentsWithZKMetadata == 0 guard does not fire either. The gauges then look plausible and are wrong, which is worse for alerting than gauges that are gone.
If you still prefer false, I am happy to defer. One extra argument for true: it makes the numSegmentsWithZKMetadata guard unnecessary.
There was a problem hiding this comment.
Discussed offline and the current path is actually not throwing an exception, the above analysis is missing that we don't set the right access option for Helix to rethrow
There was a problem hiding this comment.
In getSegmentZKMetadata we just put AccessOption.PERSISTENT instead of AccessOption.PERSISTENT + AccessOption.THROW_EXCEPTION_IFNOTEXIST, so this won't throw for getSegmentZKMetadata
There was a problem hiding this comment.
Pull request overview
Batch-reads segment ZK metadata and stats to improve SegmentStatusChecker scalability.
Changes:
- Adds index-aligned batch metadata retrieval.
- Replaces per-segment ZK reads.
- Adds batch-read and failure-path tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
ZKMetadataProvider.java |
Implements batched retrieval. |
PinotHelixResourceManager.java |
Exposes the batch API. |
SegmentStatusChecker.java |
Uses batched metadata and stats. |
SegmentStatusCheckerTest.java |
Updates and expands checker tests. |
PinotHelixResourceManagerStatelessTest.java |
Tests alignment and missing metadata. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // The segment is in the ideal state but has no ZK metadata at all | ||
| mockSegmentsZKMetadata(resourceManager, OFFLINE_TABLE_NAME, Map.of()); |
Description
The current implementation does iterative ZK metadata read for each segment. It's bad when segment count is at few hundred thousands, the zk read latency due to the queued read request causes this checker to run more than the default task frequency 5 min. There was even a
TODO: revisit the logic and reduce the ZK accessat the method itself.Change
Batch read the segment ZK metadata for the entire table, then perform the replica checks and other maths. This approach is acceptable as retention manager, realtime validation manager are all fetching them for the entire table.