Vamsi-klu commented on PR #18343: URL: https://github.com/apache/pinot/pull/18343#issuecomment-4912317000
The null-IdealState generalization to _tablesToSkipSizeAggregation and the "keep _disabledTables distinct so DISABLED_TABLE_COUNT isn't inflated" reasoning are already covered in the inline thread (noob-se7en's scope question + 1fanwang's reply), so no need to re-litigate those - nullIdealStateTest's DISABLED_TABLE_COUNT == 0 assertion confirms it. One thing not yet on the record: updateSegmentMetrics has a third early-return, `numSegments == 0`, for enabled tables with no queryable segments. That branch does not add to _tablesToSkipSizeAggregation, so those tables still fall through to updateTableSizeMetrics. It looks intentional - an enabled empty table should return size 0 rather than 404-storm - but worth confirming that path doesn't reproduce the same CompletionServiceHelper 404 noise for enabled tables that momentarily have zero segments loaded. Otherwise the fix is correct and well-scoped: the guard sits after updateSegmentMetrics populates the set, and both disabledTableTest and nullIdealStateTest fail if the guard is removed, so the regression is genuinely covered. -- 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]
