github-advanced-security[bot] commented on code in PR #18176:
URL: https://github.com/apache/druid/pull/18176#discussion_r2305495747
##########
server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java:
##########
@@ -191,500 +263,346 @@
}
@Override
- public void storeInfoFile(DataSegment segment) throws IOException
- {
- final File segmentInfoCacheFile = new File(getEffectiveInfoDir(),
segment.getId().toString());
- if (!segmentInfoCacheFile.exists()) {
- jsonMapper.writeValue(segmentInfoCacheFile, segment);
- }
- }
-
- @Override
- public void removeInfoFile(DataSegment segment)
- {
- final File segmentInfoCacheFile = new File(getEffectiveInfoDir(),
segment.getId().toString());
- if (!segmentInfoCacheFile.delete()) {
- log.warn("Unable to delete cache file[%s] for segment[%s].",
segmentInfoCacheFile, segment.getId());
- }
- }
-
- @Override
- public ReferenceCountedSegmentProvider getSegment(final DataSegment
dataSegment) throws SegmentLoadingException
+ public void storeInfoFile(final DataSegment segment) throws IOException
{
- final File segmentFiles = getSegmentFiles(dataSegment);
- final SegmentizerFactory factory = getSegmentFactory(segmentFiles);
-
- final Segment segment = factory.factorize(dataSegment, segmentFiles,
false, SegmentLazyLoadFailCallback.NOOP);
- return ReferenceCountedSegmentProvider.wrapSegment(segment,
dataSegment.getShardSpec());
- }
-
- @Override
- public ReferenceCountedSegmentProvider getBootstrapSegment(
- final DataSegment dataSegment,
- final SegmentLazyLoadFailCallback loadFailed
- ) throws SegmentLoadingException
- {
- final File segmentFiles = getSegmentFiles(dataSegment);
- final SegmentizerFactory factory = getSegmentFactory(segmentFiles);
-
- final Segment segment = factory.factorize(dataSegment, segmentFiles,
config.isLazyLoadOnStart(), loadFailed);
- return ReferenceCountedSegmentProvider.wrapSegment(segment,
dataSegment.getShardSpec());
- }
-
- private SegmentizerFactory getSegmentFactory(final File segmentFiles) throws
SegmentLoadingException
- {
- final File factoryJson = new File(segmentFiles, "factory.json");
- final SegmentizerFactory factory;
-
- if (factoryJson.exists()) {
+ final ReferenceCountingLock lock = lock(segment);
+ synchronized (lock) {
try {
- factory = jsonMapper.readValue(factoryJson, SegmentizerFactory.class);
- }
- catch (IOException e) {
- throw new SegmentLoadingException(e, "Failed to get segment facotry
for %s", e.getMessage());
- }
- } else {
- factory = new MMappedQueryableSegmentizerFactory(indexIO);
- }
- return factory;
- }
-
- /**
- * Returns the effective segment info directory based on the configuration
settings.
- * The directory is selected based on the following configurations injected
into this class:
- * <ul>
- * <li>{@link SegmentLoaderConfig#getInfoDir()} - If {@code infoDir} is
set, it is used as the info directory.</li>
- * <li>{@link SegmentLoaderConfig#getLocations()} - If the info directory
is not set, the first location from this list is used.</li>
- * <li>List of {@link StorageLocation}s injected - If both the info
directory and locations list are not set, the
- * first storage location is used.</li>
- * </ul>
- *
- * @throws DruidException if none of the configurations are set, and the
info directory cannot be determined.
- */
- private File getEffectiveInfoDir()
- {
- final File infoDir;
- if (config.getInfoDir() != null) {
- infoDir = config.getInfoDir();
- } else if (!config.getLocations().isEmpty()) {
- infoDir = new File(config.getLocations().get(0).getPath(), "info_dir");
- } else if (!locations.isEmpty()) {
- infoDir = new File(locations.get(0).getPath(), "info_dir");
- } else {
- throw DruidException.forPersona(DruidException.Persona.OPERATOR)
- .ofCategory(DruidException.Category.NOT_FOUND)
- .build("Could not determine infoDir. Make sure
'druid.segmentCache.infoDir' "
- + "or 'druid.segmentCache.locations' is set correctly.");
- }
- return infoDir;
- }
-
- private static String getSegmentDir(DataSegment segment)
- {
- return DataSegmentPusher.getDefaultStorageDir(segment, false);
- }
-
- /**
- * Checks whether a segment is already cached. It can return false even if
{@link #reserve(DataSegment)}
- * has been successful for a segment but is not downloaded yet.
- */
- boolean isSegmentCached(final DataSegment segment)
- {
- return findStoragePathIfCached(segment) != null;
- }
-
- /**
- * This method will try to find if the segment is already downloaded on any
location. If so, the segment path
- * is returned. Along with that, location state is also updated with the
segment location. Refer to
- * {@link StorageLocation#maybeReserve(String, DataSegment)} for more
details.
- * If the segment files are damaged in any location, they are removed from
the location.
- * @param segment - Segment to check
- * @return - Path corresponding to segment directory if found, null
otherwise.
- */
- @Nullable
- private File findStoragePathIfCached(final DataSegment segment)
- {
- for (StorageLocation location : locations) {
- String storageDir = getSegmentDir(segment);
- File localStorageDir = location.segmentDirectoryAsFile(storageDir);
- if (localStorageDir.exists()) {
- if (checkSegmentFilesIntact(localStorageDir)) {
- log.warn(
- "[%s] may be damaged. Delete all the segment files and pull from
DeepStorage again.",
- localStorageDir.getAbsolutePath()
- );
- cleanupCacheFiles(location.getPath(), localStorageDir);
- location.removeSegmentDir(localStorageDir, segment);
- break;
- } else {
- // Before returning, we also reserve the space. Refer to the
StorageLocation#maybeReserve documentation for details.
- location.maybeReserve(storageDir, segment);
- return localStorageDir;
+ final File segmentInfoCacheFile = new File(getEffectiveInfoDir(),
segment.getId().toString());
+ if (!segmentInfoCacheFile.exists()) {
+ jsonMapper.writeValue(segmentInfoCacheFile, segment);
}
}
+ finally {
+ unlock(segment, lock);
+ }
}
- return null;
- }
-
- /**
- * check data intact.
- * @param dir segments cache dir
- * @return true means segment files may be damaged.
- */
- private boolean checkSegmentFilesIntact(File dir)
- {
- return checkSegmentFilesIntactWithStartMarker(dir);
}
- /**
- * If there is 'downloadStartMarker' existed in localStorageDir, the
segments files might be damaged.
- * Because each time, Druid will delete the 'downloadStartMarker' file after
pulling and unzip the segments from DeepStorage.
- * downloadStartMarker existed here may mean something error during download
segments and the segment files may be damaged.
- */
- private boolean checkSegmentFilesIntactWithStartMarker(File localStorageDir)
- {
- final File downloadStartMarker = new File(localStorageDir.getPath(),
DOWNLOAD_START_MARKER_FILE_NAME);
- return downloadStartMarker.exists();
- }
-
- /**
- * Make sure segments files in loc is intact, otherwise function like
loadSegments will failed because of segment files is damaged.
- * @param segment
- * @return
- * @throws SegmentLoadingException
- */
@Override
- public File getSegmentFiles(DataSegment segment) throws
SegmentLoadingException
+ public void removeInfoFile(final DataSegment segment)
{
- final ReferenceCountingLock lock = createOrGetLock(segment);
+ final ReferenceCountingLock lock = lock(segment);
synchronized (lock) {
try {
- File segmentDir = findStoragePathIfCached(segment);
- if (segmentDir != null) {
- return segmentDir;
+ final File segmentInfoCacheFile = new File(getEffectiveInfoDir(),
segment.getId().toString());
+ if (!segmentInfoCacheFile.delete()) {
Review Comment:
## Uncontrolled data used in path expression
This path depends on a [user-provided value](1).
[Show more
details](https://github.com/apache/druid/security/code-scanning/10259)
##########
server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java:
##########
@@ -191,500 +263,346 @@
}
@Override
- public void storeInfoFile(DataSegment segment) throws IOException
- {
- final File segmentInfoCacheFile = new File(getEffectiveInfoDir(),
segment.getId().toString());
- if (!segmentInfoCacheFile.exists()) {
- jsonMapper.writeValue(segmentInfoCacheFile, segment);
- }
- }
-
- @Override
- public void removeInfoFile(DataSegment segment)
- {
- final File segmentInfoCacheFile = new File(getEffectiveInfoDir(),
segment.getId().toString());
- if (!segmentInfoCacheFile.delete()) {
- log.warn("Unable to delete cache file[%s] for segment[%s].",
segmentInfoCacheFile, segment.getId());
- }
- }
-
- @Override
- public ReferenceCountedSegmentProvider getSegment(final DataSegment
dataSegment) throws SegmentLoadingException
+ public void storeInfoFile(final DataSegment segment) throws IOException
{
- final File segmentFiles = getSegmentFiles(dataSegment);
- final SegmentizerFactory factory = getSegmentFactory(segmentFiles);
-
- final Segment segment = factory.factorize(dataSegment, segmentFiles,
false, SegmentLazyLoadFailCallback.NOOP);
- return ReferenceCountedSegmentProvider.wrapSegment(segment,
dataSegment.getShardSpec());
- }
-
- @Override
- public ReferenceCountedSegmentProvider getBootstrapSegment(
- final DataSegment dataSegment,
- final SegmentLazyLoadFailCallback loadFailed
- ) throws SegmentLoadingException
- {
- final File segmentFiles = getSegmentFiles(dataSegment);
- final SegmentizerFactory factory = getSegmentFactory(segmentFiles);
-
- final Segment segment = factory.factorize(dataSegment, segmentFiles,
config.isLazyLoadOnStart(), loadFailed);
- return ReferenceCountedSegmentProvider.wrapSegment(segment,
dataSegment.getShardSpec());
- }
-
- private SegmentizerFactory getSegmentFactory(final File segmentFiles) throws
SegmentLoadingException
- {
- final File factoryJson = new File(segmentFiles, "factory.json");
- final SegmentizerFactory factory;
-
- if (factoryJson.exists()) {
+ final ReferenceCountingLock lock = lock(segment);
+ synchronized (lock) {
try {
- factory = jsonMapper.readValue(factoryJson, SegmentizerFactory.class);
- }
- catch (IOException e) {
- throw new SegmentLoadingException(e, "Failed to get segment facotry
for %s", e.getMessage());
- }
- } else {
- factory = new MMappedQueryableSegmentizerFactory(indexIO);
- }
- return factory;
- }
-
- /**
- * Returns the effective segment info directory based on the configuration
settings.
- * The directory is selected based on the following configurations injected
into this class:
- * <ul>
- * <li>{@link SegmentLoaderConfig#getInfoDir()} - If {@code infoDir} is
set, it is used as the info directory.</li>
- * <li>{@link SegmentLoaderConfig#getLocations()} - If the info directory
is not set, the first location from this list is used.</li>
- * <li>List of {@link StorageLocation}s injected - If both the info
directory and locations list are not set, the
- * first storage location is used.</li>
- * </ul>
- *
- * @throws DruidException if none of the configurations are set, and the
info directory cannot be determined.
- */
- private File getEffectiveInfoDir()
- {
- final File infoDir;
- if (config.getInfoDir() != null) {
- infoDir = config.getInfoDir();
- } else if (!config.getLocations().isEmpty()) {
- infoDir = new File(config.getLocations().get(0).getPath(), "info_dir");
- } else if (!locations.isEmpty()) {
- infoDir = new File(locations.get(0).getPath(), "info_dir");
- } else {
- throw DruidException.forPersona(DruidException.Persona.OPERATOR)
- .ofCategory(DruidException.Category.NOT_FOUND)
- .build("Could not determine infoDir. Make sure
'druid.segmentCache.infoDir' "
- + "or 'druid.segmentCache.locations' is set correctly.");
- }
- return infoDir;
- }
-
- private static String getSegmentDir(DataSegment segment)
- {
- return DataSegmentPusher.getDefaultStorageDir(segment, false);
- }
-
- /**
- * Checks whether a segment is already cached. It can return false even if
{@link #reserve(DataSegment)}
- * has been successful for a segment but is not downloaded yet.
- */
- boolean isSegmentCached(final DataSegment segment)
- {
- return findStoragePathIfCached(segment) != null;
- }
-
- /**
- * This method will try to find if the segment is already downloaded on any
location. If so, the segment path
- * is returned. Along with that, location state is also updated with the
segment location. Refer to
- * {@link StorageLocation#maybeReserve(String, DataSegment)} for more
details.
- * If the segment files are damaged in any location, they are removed from
the location.
- * @param segment - Segment to check
- * @return - Path corresponding to segment directory if found, null
otherwise.
- */
- @Nullable
- private File findStoragePathIfCached(final DataSegment segment)
- {
- for (StorageLocation location : locations) {
- String storageDir = getSegmentDir(segment);
- File localStorageDir = location.segmentDirectoryAsFile(storageDir);
- if (localStorageDir.exists()) {
- if (checkSegmentFilesIntact(localStorageDir)) {
- log.warn(
- "[%s] may be damaged. Delete all the segment files and pull from
DeepStorage again.",
- localStorageDir.getAbsolutePath()
- );
- cleanupCacheFiles(location.getPath(), localStorageDir);
- location.removeSegmentDir(localStorageDir, segment);
- break;
- } else {
- // Before returning, we also reserve the space. Refer to the
StorageLocation#maybeReserve documentation for details.
- location.maybeReserve(storageDir, segment);
- return localStorageDir;
+ final File segmentInfoCacheFile = new File(getEffectiveInfoDir(),
segment.getId().toString());
+ if (!segmentInfoCacheFile.exists()) {
Review Comment:
## Uncontrolled data used in path expression
This path depends on a [user-provided value](1).
[Show more
details](https://github.com/apache/druid/security/code-scanning/10257)
##########
server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java:
##########
@@ -191,500 +263,346 @@
}
@Override
- public void storeInfoFile(DataSegment segment) throws IOException
- {
- final File segmentInfoCacheFile = new File(getEffectiveInfoDir(),
segment.getId().toString());
- if (!segmentInfoCacheFile.exists()) {
- jsonMapper.writeValue(segmentInfoCacheFile, segment);
- }
- }
-
- @Override
- public void removeInfoFile(DataSegment segment)
- {
- final File segmentInfoCacheFile = new File(getEffectiveInfoDir(),
segment.getId().toString());
- if (!segmentInfoCacheFile.delete()) {
- log.warn("Unable to delete cache file[%s] for segment[%s].",
segmentInfoCacheFile, segment.getId());
- }
- }
-
- @Override
- public ReferenceCountedSegmentProvider getSegment(final DataSegment
dataSegment) throws SegmentLoadingException
+ public void storeInfoFile(final DataSegment segment) throws IOException
{
- final File segmentFiles = getSegmentFiles(dataSegment);
- final SegmentizerFactory factory = getSegmentFactory(segmentFiles);
-
- final Segment segment = factory.factorize(dataSegment, segmentFiles,
false, SegmentLazyLoadFailCallback.NOOP);
- return ReferenceCountedSegmentProvider.wrapSegment(segment,
dataSegment.getShardSpec());
- }
-
- @Override
- public ReferenceCountedSegmentProvider getBootstrapSegment(
- final DataSegment dataSegment,
- final SegmentLazyLoadFailCallback loadFailed
- ) throws SegmentLoadingException
- {
- final File segmentFiles = getSegmentFiles(dataSegment);
- final SegmentizerFactory factory = getSegmentFactory(segmentFiles);
-
- final Segment segment = factory.factorize(dataSegment, segmentFiles,
config.isLazyLoadOnStart(), loadFailed);
- return ReferenceCountedSegmentProvider.wrapSegment(segment,
dataSegment.getShardSpec());
- }
-
- private SegmentizerFactory getSegmentFactory(final File segmentFiles) throws
SegmentLoadingException
- {
- final File factoryJson = new File(segmentFiles, "factory.json");
- final SegmentizerFactory factory;
-
- if (factoryJson.exists()) {
+ final ReferenceCountingLock lock = lock(segment);
+ synchronized (lock) {
try {
- factory = jsonMapper.readValue(factoryJson, SegmentizerFactory.class);
- }
- catch (IOException e) {
- throw new SegmentLoadingException(e, "Failed to get segment facotry
for %s", e.getMessage());
- }
- } else {
- factory = new MMappedQueryableSegmentizerFactory(indexIO);
- }
- return factory;
- }
-
- /**
- * Returns the effective segment info directory based on the configuration
settings.
- * The directory is selected based on the following configurations injected
into this class:
- * <ul>
- * <li>{@link SegmentLoaderConfig#getInfoDir()} - If {@code infoDir} is
set, it is used as the info directory.</li>
- * <li>{@link SegmentLoaderConfig#getLocations()} - If the info directory
is not set, the first location from this list is used.</li>
- * <li>List of {@link StorageLocation}s injected - If both the info
directory and locations list are not set, the
- * first storage location is used.</li>
- * </ul>
- *
- * @throws DruidException if none of the configurations are set, and the
info directory cannot be determined.
- */
- private File getEffectiveInfoDir()
- {
- final File infoDir;
- if (config.getInfoDir() != null) {
- infoDir = config.getInfoDir();
- } else if (!config.getLocations().isEmpty()) {
- infoDir = new File(config.getLocations().get(0).getPath(), "info_dir");
- } else if (!locations.isEmpty()) {
- infoDir = new File(locations.get(0).getPath(), "info_dir");
- } else {
- throw DruidException.forPersona(DruidException.Persona.OPERATOR)
- .ofCategory(DruidException.Category.NOT_FOUND)
- .build("Could not determine infoDir. Make sure
'druid.segmentCache.infoDir' "
- + "or 'druid.segmentCache.locations' is set correctly.");
- }
- return infoDir;
- }
-
- private static String getSegmentDir(DataSegment segment)
- {
- return DataSegmentPusher.getDefaultStorageDir(segment, false);
- }
-
- /**
- * Checks whether a segment is already cached. It can return false even if
{@link #reserve(DataSegment)}
- * has been successful for a segment but is not downloaded yet.
- */
- boolean isSegmentCached(final DataSegment segment)
- {
- return findStoragePathIfCached(segment) != null;
- }
-
- /**
- * This method will try to find if the segment is already downloaded on any
location. If so, the segment path
- * is returned. Along with that, location state is also updated with the
segment location. Refer to
- * {@link StorageLocation#maybeReserve(String, DataSegment)} for more
details.
- * If the segment files are damaged in any location, they are removed from
the location.
- * @param segment - Segment to check
- * @return - Path corresponding to segment directory if found, null
otherwise.
- */
- @Nullable
- private File findStoragePathIfCached(final DataSegment segment)
- {
- for (StorageLocation location : locations) {
- String storageDir = getSegmentDir(segment);
- File localStorageDir = location.segmentDirectoryAsFile(storageDir);
- if (localStorageDir.exists()) {
- if (checkSegmentFilesIntact(localStorageDir)) {
- log.warn(
- "[%s] may be damaged. Delete all the segment files and pull from
DeepStorage again.",
- localStorageDir.getAbsolutePath()
- );
- cleanupCacheFiles(location.getPath(), localStorageDir);
- location.removeSegmentDir(localStorageDir, segment);
- break;
- } else {
- // Before returning, we also reserve the space. Refer to the
StorageLocation#maybeReserve documentation for details.
- location.maybeReserve(storageDir, segment);
- return localStorageDir;
+ final File segmentInfoCacheFile = new File(getEffectiveInfoDir(),
segment.getId().toString());
+ if (!segmentInfoCacheFile.exists()) {
+ jsonMapper.writeValue(segmentInfoCacheFile, segment);
Review Comment:
## Uncontrolled data used in path expression
This path depends on a [user-provided value](1).
[Show more
details](https://github.com/apache/druid/security/code-scanning/10258)
--
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]
