xiangfu0 commented on code in PR #19331:
URL: https://github.com/apache/pinot/pull/19331#discussion_r4215045834
##########
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);
Review Comment:
**[MAJOR] Cleanup omits `StarTreeV2Constants.STAR_TREE_TEMP_DIR`
(`star_tree_tmp`)** — the same killed-build scenario this block targets also
leaves `star_tree_tmp` populated (`StarTreeIndexCombiner.combine()` only runs
`cleanDirectory` after a successful combine, and the later `forceMkdir` is a
no-op on an existing dir).
Consequences on the retry:
- OFF_HEAP (the server default): `OffHeapSingleTreeBuilder`'s constructor
throws `Preconditions.checkState(!_starTreeRecordFile.exists(), "Star-tree
record file: %s already exists")` (OffHeapSingleTreeBuilder.java:78) — i.e. the
retry still fails with the exact "already exists" failure class this PR set out
to fix (it self-heals only on the attempt after, because the inner catch
deletes `starTreeIndexDir`).
- ON_HEAP: worse — no exists-check, and
`StarTreeIndexCombiner.writeNullValueVectorIfExists` includes any stale
`<column>.bitmap.nullvalue` file that merely exists in `star_tree_tmp`, so a
leftover null-vector from the killed build can be silently combined into the
new index (wrong query results, no error).
Fix: also delete `new File(_segmentDirectory,
StarTreeV2Constants.STAR_TREE_TEMP_DIR)` in this cleanup block.
##########
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.
Review Comment:
**[MAJOR] The advertised "killed prior build" state is only partially
covered: a kill between the metadata save (line 278) and the index-map store
(line 280) still bricks the segment.** In that window, metadata.properties has
`STAR_TREE_COUNT`, `star_tree_index` is complete, but `star_tree_index_map` is
missing. On the next build `_separator != null`, so this cleanup is skipped;
`getSeparator()` moves `star_tree_index` into the temp dir, then
`moveFileToDirectory` of the missing map file throws `FileNotFoundException`,
and the catch block (line 206) force-deletes the temp dir — destroying the
valid `star_tree_index` — while metadata still claims star-trees exist. The
segment then fails to load entirely (`StarTreeIndexReader.load` does `new
FileInputStream(star_tree_index_map)` with no fallback) until manual metadata
surgery or re-download.
Root-cause fix: swap lines 278-281 so `StarTreeIndexMapUtils.storeToFile`
runs before the metadata save. That makes the metadata save the single commit
point, and every pre-commit crash state then lands in the `_separator == null`
path this cleanup already handles.
##########
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);
+ }
+ }
Review Comment:
**[MAJOR] (pre-existing, same function) `_starTreeCreationFailed` is only
set in the inner loop catch (lines 262-274), so a failure in the persistence
tail (lines 278-282) bypasses the restore path in `close()` and destroys the
old star-tree without any process kill.** Example: incremental rebuild
(`_separator != null`), metadata is saved with the new `STAR_TREE_COUNT` at
line 278, then `StarTreeIndexMapUtils.storeToFile` at line 280 throws (disk
full). `SegmentPreProcessor` swallows the `build()` exception expecting
`close()` to roll back, but `close()` sees `_starTreeCreationFailed == false`
and force-deletes `existing_star_tree_tmp` holding the only copy of the old
index/map — leaving `STAR_TREE_COUNT` in metadata with no `star_tree_index_map`
on disk (segment unloadable). The same flag-scope gap applies to a failure at
`forceMkdir` (line 240) before the inner try. Setting the flag for any failure
inside the try-with-resources (not just the loop) would close this.
##########
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);
Review Comment:
**[MAJOR] (pre-existing, adjacent) Symmetric kill window inside
`getSeparator()` remains uncovered and data-destroying.** A kill after the file
moves (lines 189-192) but before the metadata save (lines 201-202) leaves
metadata still listing star-trees while `star_tree_index` lives only in
`existing_star_tree_tmp`. On retry, `_separator != null` so this cleanup never
runs; `getSeparator()` throws `FileNotFoundException` moving the now-missing
`star_tree_index` from the segment root, and its catch (line 206) force-deletes
`existing_star_tree_tmp` — the only surviving copy — while metadata still
references star-trees, so subsequent loads fail. Consider reconciling in
`getSeparator()`: if the segment-root files are missing but
`existing_star_tree_tmp` holds them, restore from the temp dir instead of
deleting it.
##########
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) {
Review Comment:
**[MINOR] The healing path is unreachable once star-tree configs are removed
from (or absent in) the table config.**
`SegmentPreProcessor.needProcessStarTree` (SegmentPreProcessor.java:485)
returns false when `starTreeBuilderConfigs` is empty and `starTreeMetadataList`
is null, and `StarTreeBuilderUtils.removeStarTrees` is likewise gated on
metadata. So orphan `star_tree_index` / `star_tree_index_map` /
`existing_star_tree_tmp` from a killed build persist indefinitely for tables
that no longer request star-trees, inflating segment size and tar uploads. If
full coverage is desired, the orphan check belongs in the preprocessor path
(run regardless of whether a build is requested), not only inside `build()`.
##########
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);
Review Comment:
**[MINOR] `FileUtils.deleteQuietly` return values are ignored, so a failed
deletion is invisible.** If the stale `star_tree_index` cannot be deleted
(permissions, Windows file lock, shared-storage IO error), the WARN at line 231
still claims cleanup happened, and `StarTreeIndexCombiner`'s
`checkState(!indexFile.exists())` then throws the very "Star-tree index file
already exists" error this PR fixes — with logs misleadingly saying the stale
files were cleaned (and possibly a partial state: map deleted, index not).
Checking the boolean (`if (!FileUtils.deleteQuietly(f)) LOGGER.error(...)`) or
using `forceDelete` inside a try/catch would surface the real cause.
##########
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];
Review Comment:
**[MINOR] `INDEX_DIR.listFiles()[0]` grabs an arbitrary child of the segment
directory and only works because a default v3 segment root currently contains
exactly the `v3` subdirectory.** `File.listFiles()` order is unspecified and
the call is NPE-prone; if the driver ever leaves another entry at the root,
`[0]` may be a regular file and setup fails spuriously. It also means
`findV3Dir` is a constant no-op (`v3/v3` never exists) and the builder is
exercised with the inner v3 dir as `indexDir`, which is not the shape
production callers use. Prefer passing `INDEX_DIR` to the builder and resolving
the segment dir via `SegmentDirectoryPaths.findSegmentDirectory(INDEX_DIR)` —
the same helper `MultipleTreesBuilder` uses.
##########
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()) {
Review Comment:
**[NIT] The triple `exists()` guard exists only to gate the WARN log**
(`deleteQuietly` already no-ops on missing paths), costing up to six stat calls
on a per-segment-build path, and when the WARN fires it does not say which
artifact was stale. Simpler and more informative: delete unconditionally and
log per artifact based on `deleteQuietly`'s boolean return.
--
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]
