Jackie-Jiang commented on code in PR #19580:
URL: https://github.com/apache/pinot/pull/19580#discussion_r4031594666
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/SegmentLocalFSDirectory.java:
##########
@@ -182,12 +182,8 @@ public long getDiskSizeBytes() {
// check that v3 subdirectory exists since the format may not have been
converted
if (_segmentDirectory != null && _segmentDirectory.exists()) {
- try {
- return FileUtils.sizeOfDirectory(_segmentDirectory.toPath().toFile());
- } catch (IllegalArgumentException e) {
- LOGGER.error("Failed to read disk size for directory: {}",
_segmentDirectory.getAbsolutePath());
- return -1;
- }
+ // A concurrent reload may be rewriting this directory in place, so use
the delete-tolerant walk.
+ return
org.apache.pinot.common.utils.FileUtils.sizeOfDirectory(_segmentDirectory);
Review Comment:
Not the only one. There are 12 other directory-sizing calls in main code,
all on directories the calling thread has just finished producing, so nothing
can race with the walk:
- `RealtimeSegmentDataManager` and `SegmentProcessorFramework`: the segment
just built
- `BaseTaskExecutor` (x2) and the segment-generation runners
(`SegmentGenerationAndPushTaskExecutor`, standalone / Hadoop / Spark job
runners): the minion's local copy before/after the task
- `PinotSegmentUploadDownloadRestletResource`: the untarred upload in a temp
dir
- `PredownloadSegmentInfo`: the local segment at startup, before Helix
participation
- `MemoryEstimator` and `SegmentSizeRule`: the recommender's sample segment
I left those on commons-io on purpose. The tolerant walk trades a genuine
I/O error for a silent `0`. That is the right trade for a gauge or a REST call
racing a writer, but not for a size about to be recorded in ZK metadata or a
task metric for a directory nobody else touches. The util's Javadoc spells out
the criterion (another thread may be writing to it) so the next site can pick
correctly.
So I'd rather not ban it. Two mechanical reasons on top of the semantic one:
- A third of the remaining calls go through `FileUtils.sizeOf(File)`, the
file-or-directory overload, so a `sizeOfDirectory` ban would miss them, and a
`sizeOf` ban would also catch the legitimate tar-file `sizeOf` calls right next
to them.
- The only lever in the build is a `RegexpSingleline` rule (no
forbidden-apis plugin), which can't tell the commons-io call from the
pinot-common method of the same name unless every caller of ours uses the fully
qualified form.
If you'd rather have one rule anyway, I'm happy to do the blanket migration
of those 12 sites in a follow-up.
--
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]
