Skip to content

Skip table size aggregation for disabled tables in SegmentStatusChecker #18343

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
1fanwang wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Skip table size aggregation for disabled tables in SegmentStatusChecker #18343

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37c05b3
Skip table size aggregation for disabled tables
1fanwang Apr 27, 2026
1ca4517
Also skip table size aggregation when IdealState is null
1fanwang May 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion pinot-controller/src/main/java/org/apache/pinot/controller/helix/SegmentStatusChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,12 @@ protected void processTable(String tableNameWithType, Context context) {
TableConfig tableConfig = _pinotHelixResourceManager.getTableConfig(tableNameWithType);
updateTableConfigMetrics(tableNameWithType, tableConfig, context);
updateSegmentMetrics(tableNameWithType, tableConfig, context);
updateTableSizeMetrics(tableNameWithType);
// Skip table size aggregation for disabled tables. Servers do not load segments for disabled tables, so the
// /table/{name}/size scatter-gather would otherwise return 404 from every server and flood controller logs
// with ERROR-level entries. updateSegmentMetrics has already populated context._disabledTables.
Copy link
Copy Markdown
Contributor

@noob-se7en noob-se7en May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the null-IdealState early-return at ~L221 doesn't populate _disabledTables, so this guard still lets that path through to updateTableSizeMetrics — same 404 pattern, no? worth handling here too, or calling out as out-of-scope.

Copy link
Copy Markdown
Author

@1fanwang 1fanwang May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — the null-IdealState branch had the same shape but a different downstream symptom (getServerToSegmentsMap throws IllegalStateException, which lands in processTable's outer catch as an ERROR + stack trace per table per cycle). Generalized the guard in 1ca4517: introduced _tablesToSkipSizeAggregation as a superset of _disabledTables so both idealState == null and !idealState.isEnabled() branches add to it, and processTable checks the new set. Kept _disabledTables distinct so the DISABLED_TABLE_COUNT gauge isn't inflated by null-IdealState entries. New nullIdealStateTest covers it.

if (!context._disabledTables.contains(tableNameWithType)) {
updateTableSizeMetrics(tableNameWithType);
}
} catch (Exception e) {
LOGGER.error("Caught exception while updating segment status for table {}", tableNameWithType, e);
// Remove the metric for this table
Expand Down
15 changes: 12 additions & 3 deletions ...-controller/src/test/java/org/apache/pinot/controller/helix/SegmentStatusCheckerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,13 @@
import org.testng.annotations.Test;

import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertFalse;
Expand Down Expand Up @@ -145,7 +148,8 @@ private SegmentZKMetadata mockPushedSegmentZKMetadata(long sizeInBytes, long pus
return segmentZKMetadata;
}

private void runSegmentStatusChecker(PinotHelixResourceManager resourceManager, int waitForPushTimeInSeconds) {
private TableSizeReader runSegmentStatusChecker(PinotHelixResourceManager resourceManager,
int waitForPushTimeInSeconds) {
LeadControllerManager leadControllerManager = mock(LeadControllerManager.class);
when(leadControllerManager.isLeaderForTable(anyString())).thenReturn(true);
ControllerConf controllerConf = mock(ControllerConf.class);
Expand All @@ -156,6 +160,7 @@ private void runSegmentStatusChecker(PinotHelixResourceManager resourceManager,
tableSizeReader);
segmentStatusChecker.start();
segmentStatusChecker.run();
return tableSizeReader;
}

private void verifyControllerMetrics(String tableNameWithType, int expectedReplicationFromConfig,
Expand Down Expand Up @@ -687,7 +692,8 @@ public void noSegmentZKMetadataTest() {
}

@Test
public void disabledTableTest() {
public void disabledTableTest()
throws Exception {
IdealState idealState = new IdealState(OFFLINE_TABLE_NAME);
// disable table in idealstate
idealState.enable(false);
Expand All @@ -701,9 +707,12 @@ public void disabledTableTest() {
when(resourceManager.getAllTables()).thenReturn(List.of(OFFLINE_TABLE_NAME));
when(resourceManager.getTableIdealState(OFFLINE_TABLE_NAME)).thenReturn(idealState);

runSegmentStatusChecker(resourceManager, 0);
TableSizeReader tableSizeReader = runSegmentStatusChecker(resourceManager, 0);
assertEquals(MetricValueUtils.getGlobalGaugeValue(_controllerMetrics, ControllerGauge.DISABLED_TABLE_COUNT), 1);
verifyControllerMetricsNotExist();
// Disabled tables should not trigger the size scatter-gather: servers do not load segments for disabled tables,
// so calling /table/{name}/size would return 404 from every server and flood logs (issue #14279).
verify(tableSizeReader, never()).getTableSizeDetails(anyString(), anyInt(), anyBoolean());
}

@Test
Expand Down
Loading