gianm commented on code in PR #18683:
URL: https://github.com/apache/druid/pull/18683#discussion_r2456881503
##########
server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java:
##########
@@ -290,6 +291,27 @@ public void storeInfoFile(final DataSegment segment)
throws IOException
@Override
public void removeInfoFile(final DataSegment segment)
+ {
+ final Runnable delete = () -> deleteSegmentInfoFile(segment);
+ final SegmentCacheEntryIdentifier entryId = new
SegmentCacheEntryIdentifier(segment.getId());
+ boolean isCached = false;
+ // defer deleting until the unmount operation of the cache entry, if
possible, so that if the process stops before
+ // the segment files are deleted, they can be properly managed on startup
(since the info entry still exists)
+ for (StorageLocation location : locations) {
+ final SegmentCacheEntry cacheEntry = location.getCacheEntry(entryId);
+ if (cacheEntry != null) {
+ isCached = true;
+ cacheEntry.onUnmount.set(delete);
Review Comment:
Can this race with an actual unmounting? A lock doesn't seem to be acquired
at this point in time.
##########
server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java:
##########
@@ -421,7 +449,23 @@ private AcquireSegmentAction
acquireExistingSegment(SegmentCacheEntryIdentifier
public void load(final DataSegment dataSegment) throws
SegmentLoadingException
{
if (config.isVirtualStorage()) {
- // no-op, we'll do a load when someone asks for the segment
+ // virtual storage doesn't do anything with loading immediately, but
check to see if the segment is already cached
+ // and if so, clear out the onUnmount action
+ final ReferenceCountingLock lock = lock(dataSegment);
+ synchronized (lock) {
+ try {
+ final SegmentCacheEntryIdentifier cacheEntryIdentifier = new
SegmentCacheEntryIdentifier(dataSegment.getId());
+ for (StorageLocation location : locations) {
+ final SegmentCacheEntry cacheEntry =
location.getCacheEntry(cacheEntryIdentifier);
+ if (cacheEntry != null) {
+ cacheEntry.onUnmount.set(null);
Review Comment:
Can this race with the call in `removeInfoFile`? That one doesn't seem to
acquire a lock. Same comment for the other various calls to `set(null)`.
##########
server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java:
##########
@@ -362,6 +383,13 @@ public AcquireSegmentAction acquireSegment(final
DataSegment dataSegment) throws
);
try {
if (hold != null) {
+ // write the segment info file if it doesn't exist. this can
happen if we are loading after a drop
+ final File segmentInfoCacheFile = new
File(getEffectiveInfoDir(), dataSegment.getId().toString());
+ if (!segmentInfoCacheFile.exists()) {
+ jsonMapper.writeValue(segmentInfoCacheFile, dataSegment);
Review Comment:
Use `FileUtils.writeAtomically` to avoid leaving a partially-written file
after a crash.
--
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]
