Jackie-Jiang commented on a change in pull request #7294:
URL: https://github.com/apache/pinot/pull/7294#discussion_r688162551
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java
##########
@@ -104,24 +103,14 @@ public static ImmutableSegment load(File indexDir,
IndexLoadingConfig indexLoadi
return new EmptyIndexSegment(localSegmentMetadata);
}
- PinotConfiguration tierConfigs = indexLoadingConfig.getTierConfigs();
- PinotConfiguration segmentDirectoryLoaderConfigs = new
PinotConfiguration(tierConfigs.toMap());
-
- // Pre-process the segment on local using local SegmentDirectory
- SegmentDirectory localSegmentDirectory =
SegmentDirectoryLoaderRegistry.getLocalSegmentDirectoryLoader()
- .load(indexDir.toURI(), segmentDirectoryLoaderConfigs);
-
- // NOTE: this step may modify the segment metadata
- try (
- SegmentPreProcessor preProcessor = new
SegmentPreProcessor(localSegmentDirectory, indexLoadingConfig, schema)) {
- preProcessor.process();
- }
+ preprocessSegment(indexDir, indexLoadingConfig, schema);
Review comment:
Keep the comments here
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java
##########
@@ -169,7 +158,20 @@ public static ImmutableSegment load(File indexDir,
IndexLoadingConfig indexLoadi
ImmutableSegmentImpl segment =
new ImmutableSegmentImpl(actualSegmentDirectory, segmentMetadata,
indexContainerMap, starTreeIndexContainer);
- LOGGER.info("Successfully loaded segment {} with config: {}", segmentName,
segmentDirectoryLoaderConfigs);
+ LOGGER.info("Successfully loaded segment {} with config: {}", segmentName,
segDirConfigs);
return segment;
}
+
+ // Pre-process the segment on local using local SegmentDirectory.
+ // Please note that this step may modify the segment metadata.
+ private static void preprocessSegment(File indexDir, IndexLoadingConfig
indexLoadingConfig, Schema schema)
+ throws Exception {
+ PinotConfiguration tierConfigs = indexLoadingConfig.getTierConfigs();
+ PinotConfiguration segDirConfigs = new
PinotConfiguration(tierConfigs.toMap());
+ SegmentDirectory segDir =
+
SegmentDirectoryLoaderRegistry.getLocalSegmentDirectoryLoader().load(indexDir.toURI(),
segDirConfigs);
Review comment:
Do we need tier configs when loading from local?
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessor.java
##########
@@ -104,77 +105,11 @@ public void process()
LOGGER.warn("Skip creating default columns for segment: {} without
schema", _segmentMetadata.getName());
}
- // Create column inverted indices according to the index config.
- InvertedIndexHandler invertedIndexHandler =
- new InvertedIndexHandler(_indexDir, _segmentMetadata,
_indexLoadingConfig, segmentWriter);
- invertedIndexHandler.createInvertedIndices();
-
- // Create column range indices according to the index config.
- RangeIndexHandler rangeIndexHandler =
- new RangeIndexHandler(_indexDir, _segmentMetadata,
_indexLoadingConfig, segmentWriter);
- rangeIndexHandler.createRangeIndices();
-
- // Create text indices according to the index config.
- Set<String> textIndexColumns = _indexLoadingConfig.getTextIndexColumns();
- if (!textIndexColumns.isEmpty()) {
- TextIndexHandler textIndexHandler =
- new TextIndexHandler(_indexDir, _segmentMetadata,
textIndexColumns, segmentWriter);
- textIndexHandler.createTextIndexesOnSegmentLoad();
- }
-
- Set<String> fstIndexColumns = _indexLoadingConfig.getFSTIndexColumns();
- if (!fstIndexColumns.isEmpty()) {
- LuceneFSTIndexHandler luceneFSTIndexHandler =
- new LuceneFSTIndexHandler(_indexDir, _segmentMetadata,
fstIndexColumns, segmentWriter);
- luceneFSTIndexHandler.createFSTIndexesOnSegmentLoad();
- }
+ createMissingIndices(segmentWriter);
Review comment:
I somehow feel this refactor is not improving the readability.
We can make star-tree handling a separate method because the logic is more
complicated.
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessor.java
##########
@@ -64,33 +69,29 @@
private final SegmentDirectory _segmentDirectory;
private SegmentMetadataImpl _segmentMetadata;
- public SegmentPreProcessor(SegmentDirectory segmentDirectory,
IndexLoadingConfig indexLoadingConfig, @Nullable Schema schema)
- throws Exception {
+ public SegmentPreProcessor(SegmentDirectory segmentDirectory,
IndexLoadingConfig indexLoadingConfig,
+ @Nullable Schema schema) {
_segmentDirectory = segmentDirectory;
_indexDir = new File(segmentDirectory.getIndexDir());
_indexLoadingConfig = indexLoadingConfig;
_schema = schema;
_segmentMetadata = segmentDirectory.getSegmentMetadata();
}
+ @Override
+ public void close()
+ throws Exception {
+ _segmentDirectory.close();
+ }
+
public void process()
throws Exception {
if (_segmentMetadata.getTotalDocs() == 0) {
LOGGER.info("Skip preprocessing empty segment: {}",
_segmentMetadata.getName());
return;
}
- // Remove all the existing inverted index temp files before loading
segments.
- // NOTE: This step fixes the issue of temporary files not getting deleted
after creating new inverted indexes.
- // In this, we look for all files in the directory and remove the ones
with '.bitmap.inv.tmp' extension.
- File[] directoryListing = _indexDir.listFiles();
- String tempFileExtension =
V1Constants.Indexes.BITMAP_INVERTED_INDEX_FILE_EXTENSION + ".tmp";
- if (directoryListing != null) {
- for (File child : directoryListing) {
- if (child.getName().endsWith(tempFileExtension)) {
- FileUtils.deleteQuietly(child);
- }
- }
- }
+
+ removeInvertedIndexTempFiles();
Review comment:
Keep the comment here
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java
##########
@@ -169,7 +158,20 @@ public static ImmutableSegment load(File indexDir,
IndexLoadingConfig indexLoadi
ImmutableSegmentImpl segment =
new ImmutableSegmentImpl(actualSegmentDirectory, segmentMetadata,
indexContainerMap, starTreeIndexContainer);
- LOGGER.info("Successfully loaded segment {} with config: {}", segmentName,
segmentDirectoryLoaderConfigs);
+ LOGGER.info("Successfully loaded segment {} with config: {}", segmentName,
segDirConfigs);
return segment;
}
+
+ // Pre-process the segment on local using local SegmentDirectory.
+ // Please note that this step may modify the segment metadata.
Review comment:
Change the comment into javadoc
--
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]
