somandal commented on code in PR #17028:
URL: https://github.com/apache/pinot/pull/17028#discussion_r2441393659
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/MultipleTreesBuilder.java:
##########
@@ -175,28 +185,44 @@ public void build()
LOGGER.info("Starting building {} star-trees with configs: {} using {}
builder", numStarTrees, _builderConfigs,
_buildMode);
- try (StarTreeIndexCombiner indexCombiner = new StarTreeIndexCombiner(
- new File(_segmentDirectory, StarTreeV2Constants.INDEX_FILE_NAME))) {
+ File starTreeV2IndexFile = new File(_segmentDirectory,
StarTreeV2Constants.INDEX_FILE_NAME);
+ try (StarTreeIndexCombiner indexCombiner = new
StarTreeIndexCombiner(starTreeV2IndexFile)) {
File starTreeIndexDir = new File(_segmentDirectory,
StarTreeV2Constants.STAR_TREE_TEMP_DIR);
FileUtils.forceMkdir(starTreeIndexDir);
_metadataProperties.addProperty(MetadataKey.STAR_TREE_COUNT,
numStarTrees);
List<List<Pair<IndexKey, IndexValue>>> indexMaps = new
ArrayList<>(numStarTrees);
// Build all star-trees
- for (int i = 0; i < numStarTrees; i++) {
- StarTreeV2BuilderConfig builderConfig = _builderConfigs.get(i);
- Configuration metadataProperties =
_metadataProperties.subset(MetadataKey.getStarTreePrefix(i));
- if (_separator != null &&
handleExistingStarTreeAddition(starTreeIndexDir, metadataProperties,
builderConfig)) {
- // Used existing tree
- LOGGER.info("Reused existing star-tree: {}",
builderConfig.toString());
- reusedStarTrees++;
- } else {
- try (SingleTreeBuilder singleTreeBuilder =
getSingleTreeBuilder(builderConfig, starTreeIndexDir, _segment,
- metadataProperties, _buildMode)) {
- singleTreeBuilder.build();
+ try {
+ for (int i = 0; i < numStarTrees; i++) {
+ StarTreeV2BuilderConfig builderConfig = _builderConfigs.get(i);
+ Configuration metadataProperties =
_metadataProperties.subset(MetadataKey.getStarTreePrefix(i));
+ if (_separator != null &&
handleExistingStarTreeAddition(starTreeIndexDir, metadataProperties,
+ builderConfig)) {
+ // Used existing tree
+ LOGGER.info("Reused existing star-tree: {}",
builderConfig.toString());
+ reusedStarTrees++;
+ } else {
+ try (SingleTreeBuilder singleTreeBuilder =
getSingleTreeBuilder(builderConfig, starTreeIndexDir, _segment,
+ metadataProperties, _buildMode)) {
+ singleTreeBuilder.build();
+ }
}
+ indexMaps.add(indexCombiner.combine(builderConfig,
starTreeIndexDir));
+ }
+ } catch (Exception e) {
+ // Clean-up some of the files created before throwing exception back
to caller
+ // No need to undo changes to _metadataProperties as changes weren't
saved to the file
+ LOGGER.error("Failed to build star-trees, cleaning up the index file
and temp directory if they exist");
+ _starTreeCreationFailed = true;
+ if (starTreeV2IndexFile.exists()) {
+ FileUtils.forceDelete(starTreeV2IndexFile);
+ }
+ if (starTreeIndexDir.exists()) {
+ FileUtils.forceDelete(starTreeIndexDir);
}
- indexMaps.add(indexCombiner.combine(builderConfig, starTreeIndexDir));
+ _metadataProperties.clearProperty(MetadataKey.STAR_TREE_COUNT);
+ throw e;
}
Review Comment:
we'd fail segment build by throwing an exception prior to this. so even
though the segment is inconsistent, it wouldn't technically be built or
available to query
--
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]
