FrankChen021 commented on code in PR #20338:
URL: https://github.com/apache/druid/pull/20338#discussion_r4015561578
##########
server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java:
##########
@@ -347,6 +355,17 @@ private void addFilesToCachedSegments(
}
if (cacheEntry.checkExists(location.getPath())) {
+ // A complete layout on disk while partial downloads are enabled is a
leftover from before they were turned
+ // on. Convert it to a partial layout now rather than leaving it to be
evicted the first time a partial-load
+ // rule targets the segment: that eviction discards a warm cache and
cannot be undone if the rule's load then
+ // fails. Converting keeps the bytes, and it means a complete entry
can no longer exist for a range-readable
+ // segment by the time anything applies a rule.
+ if (convertCompleteLayoutToPartial(segment, location)) {
Review Comment:
[P1] Do not report a lost layout as cached
**Finding:** When the conversion has already moved/deleted the complete
directory but the final move from staging fails, convertCompleteLayoutToPartial
returns false. This caller treats every false result as an intact complete
cache hit, sets removeInfo to false, and returns the segment from
getCachedSegments even though neither the complete directory nor a partial
directory is usable. bootstrap then finds no reservation and returns the
segment without mounting it, so startup can announce an unavailable segment
instead of retrying or failing the load.
**Suggestion:** Return a distinct outcome for a conversion that lost the
source layout, or revalidate the installed layout here and leave the info entry
uncached / fail bootstrap when no usable directory remains.
##########
server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java:
##########
@@ -370,6 +389,101 @@ private void addFilesToCachedSegments(
}
}
+ /**
+ * Rewrite a complete on-disk cache layout for {@code segment} as a partial
(bundle) layout, returning whether it
+ * did. Called from {@link #getCachedSegments}, which runs before anything
is reserved, mounted, or serving, so
+ * there is no live entry to unmap and no cache key to swap. This method
should never be called on a 'live' entry.
+ * <p>
+ * The conversion reads the local complete file as if it were deep storage,
via a
+ * {@link DirectoryBackedRangeReader}, and materializes the bundle layout
from it. Nothing is fetched remotely.
+ * Deep storage is still probed for range-readability, because a partial
entry that cannot range-read deep storage
+ * could never fetch a bundle it is missing later, and {@link
#reservePartialForBootstrap} reclaims such a layout
+ * on the next boot anyway.
+ * <p>
+ * Transient disk cost is one segment: the complete file has to stay
readable until the bundles are written. If
+ * anything fails, the complete layout is left exactly as it was and the
segment loads the old way, so a location
+ * without the headroom degrades instead of breaking. Staging happens under
{@link #CONVERT_PATH}, swept at
+ * startup, so a crash leaves garbage rather than a half-converted segment
directory. The one lossy window is a
+ * crash between removing the complete layout and moving the converted one
into place, which costs a re-download
+ * of that segment.
+ */
+ private boolean convertCompleteLayoutToPartial(final DataSegment segment,
final StorageLocation location)
+ {
+ if (!config.isVirtualStorage() ||
!config.isVirtualStoragePartialDownloadsEnabled()) {
+ return false;
+ }
+ final File completeDir = new File(location.getPath(),
segment.getId().toString());
+ if (!new File(completeDir, IndexIO.V10_FILE_NAME).exists()) {
+ // Only a V10 segment file can be re-sliced into bundles.
+ return false;
+ }
+ try {
+ if (tryOpenRangeReader(segment) == null) {
+ return false;
+ }
+ }
+ catch (Exception e) {
+ log.warn(
+ e,
+ "Failed to open a range reader for segment[%s]; leaving its complete
cache layout alone",
+ segment.getId()
+ );
+ return false;
+ }
+
+ final File stagingDir = new File(new File(location.getPath(),
CONVERT_PATH), segment.getId().toString());
+ boolean completeLayoutRemoved = false;
+ try {
+ if (stagingDir.exists()) {
+ FileUtils.deleteDirectory(stagingDir);
+ }
+ try (PartialSegmentFileMapperV10 mapper =
PartialSegmentFileMapperV10.create(
+ new DirectoryBackedRangeReader(completeDir),
+ jsonMapper,
+ stagingDir,
+ IndexIO.V10_FILE_NAME,
+ List.of(),
+ PartialSegmentDownloadListener.NOOP,
+ config.getVirtualStorageCoalesceGapBytes(),
+ config.getVirtualStorageMaxFetchRunBytes()
+ )) {
+ mapper.ensureAllDownloaded();
+ }
+ atomicMoveAndDeleteCacheEntryDirectory(completeDir);
+ completeLayoutRemoved = true;
+ Files.move(stagingDir.toPath(), completeDir.toPath(),
StandardCopyOption.ATOMIC_MOVE);
Review Comment:
[P1] Preserve custom segment factory metadata
**Finding:** The staging directory contains only the mapper header and
container files, so replacing completeDir here deletes auxiliary files such as
factory.json. CompleteSegmentCacheEntry.mount explicitly reads factory.json to
select a non-default SegmentizerFactory, while the partial bootstrap path
constructs a default PartialQueryableIndexSegment. A range-readable V10 segment
that was built with a custom factory can therefore silently lose behavior (for
example, a broadcast-joinable segment) on the first restart after partial
downloads are enabled.
**Suggestion:** Preserve the complete layout's required auxiliary files and
carry their semantics into the partial path, or exclude layouts with a
non-default factory from this migration and cover that case.
##########
server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java:
##########
@@ -370,6 +389,101 @@ private void addFilesToCachedSegments(
}
}
+ /**
+ * Rewrite a complete on-disk cache layout for {@code segment} as a partial
(bundle) layout, returning whether it
+ * did. Called from {@link #getCachedSegments}, which runs before anything
is reserved, mounted, or serving, so
+ * there is no live entry to unmap and no cache key to swap. This method
should never be called on a 'live' entry.
+ * <p>
+ * The conversion reads the local complete file as if it were deep storage,
via a
+ * {@link DirectoryBackedRangeReader}, and materializes the bundle layout
from it. Nothing is fetched remotely.
+ * Deep storage is still probed for range-readability, because a partial
entry that cannot range-read deep storage
+ * could never fetch a bundle it is missing later, and {@link
#reservePartialForBootstrap} reclaims such a layout
+ * on the next boot anyway.
+ * <p>
+ * Transient disk cost is one segment: the complete file has to stay
readable until the bundles are written. If
+ * anything fails, the complete layout is left exactly as it was and the
segment loads the old way, so a location
+ * without the headroom degrades instead of breaking. Staging happens under
{@link #CONVERT_PATH}, swept at
+ * startup, so a crash leaves garbage rather than a half-converted segment
directory. The one lossy window is a
+ * crash between removing the complete layout and moving the converted one
into place, which costs a re-download
+ * of that segment.
+ */
+ private boolean convertCompleteLayoutToPartial(final DataSegment segment,
final StorageLocation location)
+ {
+ if (!config.isVirtualStorage() ||
!config.isVirtualStoragePartialDownloadsEnabled()) {
+ return false;
+ }
+ final File completeDir = new File(location.getPath(),
segment.getId().toString());
+ if (!new File(completeDir, IndexIO.V10_FILE_NAME).exists()) {
Review Comment:
[P1] Honor the incomplete-download marker
**Finding:** CompleteSegmentCacheEntry.mount uses downloadStartMarker to
detect a directory that may have been damaged by an interrupted or failed full
download and deliberately deletes and re-fetches it. This migration checks only
for druid.segment, so it can read such a directory, write a partial layout, and
discard the marker before the normal corruption-recovery path runs. If the main
file is present but contains damaged bytes, the mapper's header/range reads do
not restore that integrity check and the damaged cache can be announced as
usable.
**Suggestion:** Skip conversion whenever isPossiblyCorrupted(completeDir) is
true and let the existing complete-entry mount path reclaim and re-download the
marked directory.
--
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]
