jihoonson commented on a change in pull request #8114: Fix race between
canHandle() and addSegment() in StorageLocation
URL: https://github.com/apache/incubator-druid/pull/8114#discussion_r306573864
##########
File path:
server/src/main/java/org/apache/druid/segment/loading/StorageLocation.java
##########
@@ -72,65 +83,81 @@ public long getMaxSize()
}
/**
- * Add a new file to this location. The given file argument must be a file
rather than directory.
+ * Remove a segment file from this location. The given file argument must be
a file rather than directory.
*/
- public synchronized void addFile(File file)
+ public synchronized void removeFile(File file)
{
- if (file.isDirectory()) {
- throw new ISE("[%s] must be a file. Use a");
- }
- if (files.add(file)) {
- currSize += FileUtils.sizeOf(file);
+ if (files.remove(file)) {
+ currSize -= FileUtils.sizeOf(file);
+ } else {
+ log.warn("File[%s] is not found under this location[%s]", file, path);
}
}
/**
- * Add a new segment dir to this location. The segment size is added to
currSize.
+ * Remove a segment dir from this location. The segment size is subtracted
from currSize.
*/
- public synchronized void addSegmentDir(File segmentDir, DataSegment segment)
+ public synchronized void removeSegmentDir(File segmentDir, DataSegment
segment)
Review comment:
I already looked into that way. The thing is, the way to use
`StorageLocation` is different in `IntermediaryDataManager` and
`SegmentLoaderLocalCacheManager`. In `IntermediaryDataManager`, the compressed
segment files are stored and registered in `StorageLocation`. In
`SegmentLoaderLocalCacheManager`, the uncompressed segment directories are
registered in `StorageLocation`. This led to use different size computations in
`removeFile(File)` and `removeSegmentDir(File, DataSegment)`. I wanted to make
sure that the caller must be aware of what it's registering and call the right
method.
I guess we could have an abstract `StorageLocation` class and its two
implementations for different use cases. But I'm not sure it's super beneficial
at this point because `StorageLocation` is still pretty simple.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
