Disable recursion in PinotFS copy #8162
Conversation
All uses of PinotFS copy API involve copying a tarred segment and untarring it. So, copying a directory recursively will not work (the untar will fail). It also results in wastage of effort in copying across file systems. Also disabled the file scheme in during segment upload on the controller, since the URL based upload is meant to provide an external URL to be picked up by the controller.
...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
Outdated
Show resolved
Hide resolved
...t-file-system/pinot-hdfs/src/main/java/org/apache/pinot/plugin/filesystem/HadoopPinotFS.java
Outdated
Show resolved
Hide resolved
pinot-spi/src/test/java/org/apache/pinot/spi/filesystem/LocalPinotFSTest.java
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #8162 +/- ##
============================================
- Coverage 71.39% 71.37% -0.03%
+ Complexity 4306 4303 -3
============================================
Files 1620 1624 +4
Lines 83917 84302 +385
Branches 12545 12637 +92
============================================
+ Hits 59915 60168 +253
- Misses 19916 20014 +98
- Partials 4086 4120 +34
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -85,7 +85,7 @@ public boolean doMove(URI srcUri, URI dstUri) | |||
| @Override | |||
| public boolean copy(URI srcUri, URI dstUri) | |||
| throws IOException { | |||
| copy(toFile(srcUri), toFile(dstUri)); | |||
| copy(toFile(srcUri), toFile(dstUri), false); | |||
There was a problem hiding this comment.
the behavior of this API is not the same in LocalPinotFs vs. S3PinotFs.
should we also expose a copy(URI srcUri, URI dstUri, boolean isRecursive) ?
There was a problem hiding this comment.
Why?
Wen there is a need, we can add it. In general, discourage recursive copy, since we know not what is being copied.
There was a problem hiding this comment.
the fact is that after this PR
LocalPinotFS.copy(srcUri, dstUri) throws exception if srcUri is a directory.
S3PinotFS.copy(srcUri, dstUri) return true and a recursive copy happened if srcUri is a directory.
this basically means users who needs to do a recursive copying would need to know exactly what underlying implementation of the PinotFS it has. i am not sure that's the right abstraction for this API.
my suggestion is to
- make the current S3PintoFS.copy(srcUri, dstUri) to also throw exception when the URI is a folder. thus make it consistent with LocalFS
- move the current recursive copy implementation in S3 to a new API: PinotFS.copy(srcUri, dstUri, recursive = true).
All uses of PinotFS copy API involve copying a tarred segment and
untarring it. So, copying a directory recursively will not work (the
untar will fail). It also results in wastage of effort in copying
across file systems.
Also disabled the file scheme in during segment upload on the controller,
since the URL based upload is meant to provide an external URL to be
picked up by the controller.
Description
Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
backward-incompat, and complete the section below on Release Notes)Does this PR fix a zero-downtime upgrade introduced earlier?
backward-incompat, and complete the section below on Release Notes)Does this PR otherwise need attention when creating release notes? Things to consider:
release-notesand complete the section on Release Notes)Release Notes
Documentation