yashmayya commented on code in PR #17028:
URL: https://github.com/apache/pinot/pull/17028#discussion_r2437439477
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentIndexCreationDriverImpl.java:
##########
@@ -542,6 +541,10 @@ private void buildStarTreeV2IfNecessary(File indexDir)
MultipleTreesBuilder builder = new
MultipleTreesBuilder(starTreeIndexConfigs, enableDefaultStarTree, indexDir,
buildMode)) {
builder.build();
+ } catch (Exception e) {
+ String tableNameWithType = _config.getTableConfig().getTableName();
+ LOGGER.error("Failed to build star-tree index for table: {},
skipping", tableNameWithType, e);
+ MinionMetrics.get().addMeteredTableValue(tableNameWithType,
MinionMeter.STAR_TREE_INDEX_BUILD_FAILURES, 1);
Review Comment:
Why are we using minion metrics here? Isn't this segment creation driver
also used when converting mutable / consuming segments to immutable ones for
realtime tables?
##########
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:
Just so I understand correctly - before these changes, any failure here
could leave the startree index file in an inconsistent state?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/MultipleTreesBuilder.java:
##########
@@ -243,6 +269,28 @@ private static SingleTreeBuilder
getSingleTreeBuilder(StarTreeV2BuilderConfig bu
public void close() {
if (_separatorTempDir != null) {
try {
+ try {
Review Comment:
This looks a little strange - can we move the outer try to after the new try
block (i.e., just enclosing the separator tmp directory deletion step)?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/MultipleTreesBuilder.java:
##########
@@ -243,6 +269,28 @@ private static SingleTreeBuilder
getSingleTreeBuilder(StarTreeV2BuilderConfig bu
public void close() {
if (_separatorTempDir != null) {
try {
+ try {
+ if (_starTreeCreationFailed) {
+ LOGGER.error("Star-tree index creation failed, trying to reset the
older star-tree index and metadata");
+ FileUtils.moveFileToDirectory(new File(_separatorTempDir,
StarTreeV2Constants.INDEX_FILE_NAME),
+ _segmentDirectory,
+ false);
+ FileUtils.moveFileToDirectory(new File(_separatorTempDir,
StarTreeV2Constants.INDEX_MAP_FILE_NAME),
+ _segmentDirectory, false);
+
+ // Copy back the older star-tree related metadata
+ Iterator<String> keys = _existingStarTreeMetadata.getKeys();
+ while (keys.hasNext()) {
+ String key = keys.next();
+ Object value = _existingStarTreeMetadata.getProperty(key);
+ _metadataProperties.addProperty(MetadataKey.STAR_TREE_PREFIX +
key, value);
+ }
+ CommonsConfigurationUtils.saveToFile(_metadataProperties,
+ new File(_segmentDirectory,
V1Constants.MetadataKeys.METADATA_FILE_NAME));
+ }
+ } catch (Exception e) {
+ LOGGER.error("Could not reset the star-tree index state to the
previous one", e);
Review Comment:
Should we cleanup everything in this case instead of leaving things in a
potentially inconsistent state?
--
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]
