From 7320a7a0795b44dc6df1766f76afc4d2f6a9ff1b Mon Sep 17 00:00:00 2001 From: Chaitanya Deepthi Date: Fri, 21 Aug 2026 14:09:29 -0700 Subject: [PATCH] Clean up stale star-tree index leftover from killed prior build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MultipleTreesBuilder.build() opens StarTreeIndexCombiner which requires the target star_tree_index file to NOT exist (StarTreeIndexCombiner:47). A previous build that was hard-killed (JVM crash, container OOM, thread hard-interrupt) can leave a partial star_tree_index at the segment root without a matching STAR_TREE_COUNT in segment metadata, because the metadata save at line 265 runs only after all trees complete. The catch block at line 249-262 cleans up on Java exceptions but never runs on process kill. On the next preprocess, segment metadata says "no star-tree" so `_separator == null`, the incremental move-aside step is skipped, and the combiner opens on the leftover file, hitting the checkState. The failure is not self-healing: the leftover keeps blocking every retry. Fix: when `_separator == null` (fresh build, no matching metadata), delete any stale star_tree_index / star_tree_index_map / EXISTING_STAR_TREE_TEMP_DIR before opening the combiner. Metadata is authoritative — with `_separator == null`, any on-disk star-tree artifact is orphaned. Log at WARN so operators see the recovery. The incremental path (`_separator != null`) is untouched. Adds MultipleTreesBuilderStaleCleanupTest covering: - stale star_tree_index at segment root -> cleaned up + fresh tree built - stale EXISTING_STAR_TREE_TEMP_DIR -> cleaned up + fresh tree built --- .../v2/builder/MultipleTreesBuilder.java | 13 ++ .../MultipleTreesBuilderStaleCleanupTest.java | 163 ++++++++++++++++++ 2 files changed, 176 insertions(+) create mode 100644 pinot-segment-local/src/test/java/org/apache/pinot/segment/local/startree/v2/builder/MultipleTreesBuilderStaleCleanupTest.java diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/MultipleTreesBuilder.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/MultipleTreesBuilder.java index f971128e13f7..bb94e4f47e9c 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/MultipleTreesBuilder.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/MultipleTreesBuilder.java @@ -222,6 +222,19 @@ public void build() _buildMode); File starTreeV2IndexFile = new File(_segmentDirectory, StarTreeV2Constants.INDEX_FILE_NAME); + // When _separator is null, metadata has no star-tree, so any leftover files on disk are orphaned + // (e.g. from a previous build killed before the metadata save). Delete them so the combiner opens. + if (_separator == null) { + File starTreeV2IndexMapFile = new File(_segmentDirectory, StarTreeV2Constants.INDEX_MAP_FILE_NAME); + File existingSeparatorDir = new File(_segmentDirectory, StarTreeV2Constants.EXISTING_STAR_TREE_TEMP_DIR); + if (starTreeV2IndexFile.exists() || starTreeV2IndexMapFile.exists() || existingSeparatorDir.exists()) { + LOGGER.warn("Cleaning up stale star-tree artifacts in {} from a prior incomplete build", + _segmentDirectory); + FileUtils.deleteQuietly(starTreeV2IndexFile); + FileUtils.deleteQuietly(starTreeV2IndexMapFile); + FileUtils.deleteQuietly(existingSeparatorDir); + } + } try (StarTreeIndexCombiner indexCombiner = new StarTreeIndexCombiner(starTreeV2IndexFile)) { File starTreeIndexDir = new File(_segmentDirectory, StarTreeV2Constants.STAR_TREE_TEMP_DIR); FileUtils.forceMkdir(starTreeIndexDir); diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/startree/v2/builder/MultipleTreesBuilderStaleCleanupTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/startree/v2/builder/MultipleTreesBuilderStaleCleanupTest.java new file mode 100644 index 000000000000..877807fe26d1 --- /dev/null +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/startree/v2/builder/MultipleTreesBuilderStaleCleanupTest.java @@ -0,0 +1,163 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.segment.local.startree.v2.builder; + +import java.io.File; +import java.nio.charset.StandardCharsets; +import java.util.List; +import org.apache.commons.io.FileUtils; +import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentLoader; +import org.apache.pinot.segment.local.segment.creator.impl.SegmentIndexCreationDriverImpl; +import org.apache.pinot.segment.local.segment.readers.GenericRowRecordReader; +import org.apache.pinot.segment.local.startree.StarTreeBuilderUtils; +import org.apache.pinot.segment.spi.ImmutableSegment; +import org.apache.pinot.segment.spi.creator.SegmentGeneratorConfig; +import org.apache.pinot.segment.spi.index.startree.StarTreeV2Constants; +import org.apache.pinot.spi.config.table.StarTreeIndexConfig; +import org.apache.pinot.spi.config.table.TableConfig; +import org.apache.pinot.spi.config.table.TableType; +import org.apache.pinot.spi.data.FieldSpec; +import org.apache.pinot.spi.data.Schema; +import org.apache.pinot.spi.data.readers.GenericRow; +import org.apache.pinot.spi.utils.ReadMode; +import org.apache.pinot.spi.utils.builder.TableConfigBuilder; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +import static org.testng.Assert.assertFalse; +import static org.testng.Assert.assertTrue; + + +/// Unit test for {@link MultipleTreesBuilder#build()} verifying that a stale on-disk `star_tree_index` +/// (leftover from a previous build attempt that was killed before completing) is cleaned up on the next +/// build instead of triggering the `Star-tree index file already exists` IllegalStateException from +/// {@link StarTreeIndexCombiner}. +public class MultipleTreesBuilderStaleCleanupTest { + private static final File TEMP_DIR = + new File(FileUtils.getTempDirectory(), "MultipleTreesBuilderStaleCleanupTest"); + private static final File INDEX_DIR = new File(TEMP_DIR, "testSegment"); + + @BeforeMethod + public void setUp() + throws Exception { + FileUtils.deleteQuietly(TEMP_DIR); + FileUtils.forceMkdir(TEMP_DIR); + buildBaseSegment(); + } + + @AfterMethod + public void tearDown() { + FileUtils.deleteQuietly(TEMP_DIR); + } + + /// A previous build that was hard-killed (JVM crash / SIGKILL / OOM) leaves behind a partial + /// `star_tree_index` at the segment root without a matching `STAR_TREE_COUNT` in segment metadata. + /// The next build must clean it up and succeed rather than fail with `Star-tree index file already exists`. + @Test + public void staleIndexFileFromKilledBuildIsCleanedUp() + throws Exception { + File segmentDir = INDEX_DIR.listFiles()[0]; + File v3Dir = findV3Dir(segmentDir); + File staleIndex = new File(v3Dir, StarTreeV2Constants.INDEX_FILE_NAME); + File staleIndexMap = new File(v3Dir, StarTreeV2Constants.INDEX_MAP_FILE_NAME); + // Simulate the killed-build leftover: a file at the segment root that segment metadata does not know about. + FileUtils.writeStringToFile(staleIndex, "stale bytes from a prior killed build", StandardCharsets.UTF_8); + assertTrue(staleIndex.exists()); + + // Kick off a fresh build. Without the fix this throws `IllegalStateException: Star-tree index file already exists`. + List builderConfigs = createBuilderConfigs(segmentDir); + try (MultipleTreesBuilder builder = new MultipleTreesBuilder(builderConfigs, segmentDir, + MultipleTreesBuilder.BuildMode.OFF_HEAP)) { + builder.build(); + } + + // Post-build: the stale bytes are gone, a real index file and map file exist. + assertTrue(staleIndex.isFile(), "star_tree_index should exist after a successful build"); + assertTrue(staleIndexMap.isFile(), "star_tree_index_map should exist after a successful build"); + assertTrue(staleIndex.length() > "stale bytes from a prior killed build".length(), + "star_tree_index should be a real serialized tree, not the stale contents"); + } + + /// A lingering EXISTING_STAR_TREE_TEMP_DIR from a killed incremental build (previous run set files aside + /// but never restored them) is also cleaned up on the next fresh build. + @Test + public void staleSeparatorTempDirFromKilledIncrementalIsCleanedUp() + throws Exception { + File segmentDir = INDEX_DIR.listFiles()[0]; + File v3Dir = findV3Dir(segmentDir); + File staleSeparatorDir = new File(v3Dir, StarTreeV2Constants.EXISTING_STAR_TREE_TEMP_DIR); + FileUtils.forceMkdir(staleSeparatorDir); + FileUtils.writeStringToFile(new File(staleSeparatorDir, "leftover.bin"), "old", StandardCharsets.UTF_8); + assertTrue(staleSeparatorDir.isDirectory()); + + List builderConfigs = createBuilderConfigs(segmentDir); + try (MultipleTreesBuilder builder = new MultipleTreesBuilder(builderConfigs, segmentDir, + MultipleTreesBuilder.BuildMode.OFF_HEAP)) { + builder.build(); + } + + // The temp dir has been cleaned up (either by our pre-build cleanup, or by the normal build flow). + assertFalse(staleSeparatorDir.exists(), + "stale EXISTING_STAR_TREE_TEMP_DIR should have been removed by the fresh build"); + } + + private void buildBaseSegment() + throws Exception { + Schema schema = new Schema.SchemaBuilder() + .addSingleValueDimension("stringCol", FieldSpec.DataType.STRING) + .addMetric("longCol", FieldSpec.DataType.LONG) + .build(); + TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE) + .setTableName("testTable") + .build(); + SegmentGeneratorConfig config = new SegmentGeneratorConfig(tableConfig, schema); + config.setOutDir(TEMP_DIR.getAbsolutePath()); + config.setSegmentName("testSegment"); + List rows = List.of(makeRow("A", 1L), makeRow("B", 2L), makeRow("C", 3L)); + SegmentIndexCreationDriverImpl driver = new SegmentIndexCreationDriverImpl(); + driver.init(config, new GenericRowRecordReader(rows)); + driver.build(); + } + + private GenericRow makeRow(String s, long l) { + GenericRow row = new GenericRow(); + row.putValue("stringCol", s); + row.putValue("longCol", l); + return row; + } + + private List createBuilderConfigs(File segmentDir) + throws Exception { + StarTreeIndexConfig starTreeConfig = + new StarTreeIndexConfig(List.of("stringCol"), null, List.of("SUM__longCol"), null, 1000); + ImmutableSegment segment = ImmutableSegmentLoader.load(segmentDir, ReadMode.mmap); + try { + return StarTreeBuilderUtils.generateBuilderConfigs(List.of(starTreeConfig), false, + segment.getSegmentMetadata()); + } finally { + segment.destroy(); + } + } + + private static File findV3Dir(File segmentDir) { + File v3 = new File(segmentDir, "v3"); + return v3.isDirectory() ? v3 : segmentDir; + } +}