yashmayya commented on code in PR #19252:
URL: https://github.com/apache/pinot/pull/19252#discussion_r3807960479
##########
pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java:
##########
@@ -639,8 +640,36 @@ public static Schema
getTableSchema(ZkHelixPropertyStore<ZNRecord> propertyStore
return getTableSchema(propertyStore, tableConfig.getTableName());
}
+ /// Reads the ZK metadata of the named segments of the given table in a
single batched request, and returns it
+ /// index-aligned with `segmentNames`. An entry is `null` when the segment's
znode could not be read, either because
+ /// it does not exist or because the read failed; the two are not
distinguished.
+ ///
+ /// When `stats` is non-null it is filled with the [Stat] of each segment's
znode, also index-aligned with
+ /// `segmentNames`, and `null` wherever the record is `null`.
+ public static List<SegmentZKMetadata>
getSegmentsZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore,
+ String tableNameWithType, List<String> segmentNames, @Nullable
List<Stat> stats) {
+ int numSegments = segmentNames.size();
+ List<String> paths = new ArrayList<>(numSegments);
+ for (String segmentName : segmentNames) {
+ paths.add(constructPropertyStorePathForSegment(tableNameWithType,
segmentName));
+ }
+ List<ZNRecord> znRecords = propertyStore.get(paths, stats,
AccessOption.PERSISTENT, false);
Review 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)`:
```java
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.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
