Copilot commented on code in PR #17055:
URL: https://github.com/apache/pinot/pull/17055#discussion_r2453317152
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/loader/SegmentDirectoryLoaderContext.java:
##########
@@ -89,6 +91,10 @@ public Map<String, Map<String, String>>
getInstanceTierConfigs() {
return _instanceTierConfigs;
}
+ public Map<String, String> getSegmentCustomConfigs() {
+ return _segmentCustomConfigs;
+ }
Review Comment:
The getter returns the internal mutable map directly, which could allow
external modification of the internal state. Return an unmodifiable view using
`Collections.unmodifiableMap(_segmentCustomConfigs)` or a defensive copy to
prevent external modifications.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/SegmentLocalFSDirectory.java:
##########
@@ -75,15 +77,29 @@ public SegmentLocalFSDirectory(File directory) {
_segmentDirectory = null;
_segmentLock = new SegmentLock();
_readMode = null;
+ _segmentDirectoryLoaderContext = null;
}
public SegmentLocalFSDirectory(File directory, ReadMode readMode)
throws IOException, ConfigurationException {
- this(directory, new SegmentMetadataImpl(directory), readMode);
+ this(directory, new SegmentMetadataImpl(directory), null, readMode);
+ }
+
+ public SegmentLocalFSDirectory(File directory, @Nullable
SegmentDirectoryLoaderContext segmentDirectoryLoaderContext,
+ ReadMode readMode)
+ throws IOException, ConfigurationException {
+ this(directory, new SegmentMetadataImpl(directory),
segmentDirectoryLoaderContext, readMode);
+ }
Review Comment:
The `@Nullable` annotation is missing on the `segmentDirectoryLoaderContext`
parameter in the constructor at line 100-101. For consistency with the new
constructor at line 88, add `@Nullable` annotation to the parameter at line 101.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java:
##########
@@ -116,9 +128,10 @@ public static ImmutableSegment load(File indexDir,
IndexLoadingConfig indexLoadi
return new EmptyIndexSegment(segmentMetadata);
}
if (needPreprocess) {
- preprocess(indexDir, indexLoadingConfig, segmentOperationsThrottler);
+ preprocess(indexDir, indexLoadingConfig, segmentOperationsThrottler,
zkMetadata);
}
String segmentName = segmentMetadata.getName();
+ Map<String, String> segmentCustomConfigs = zkMetadata != null ?
zkMetadata.getCustomMap() : new HashMap<>();
Review Comment:
This logic is duplicated in at least three places (lines 134, 317, and 1617
in BaseTableDataManager). Consider extracting this into a helper method like
`getSegmentCustomConfigs(SegmentZKMetadata zkMetadata)` that returns an empty
map when zkMetadata is null.
--
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]
