add copy recursive API to pinotFS

[walterddr]

Description

After #8162, behavior for pinotFS.copy is different between cloud and local implementation.

• 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

Proposal

• make the current S3PintoFS.copy(srcUri, dstUri) to also throw exception when the URI is a folder. thus make it consistent with LocalFS
  • this is achieved via a default copy function that rejects directory URIs.
• move the current recursive copy implementation in S3 to a new API: PinotFS.copyDir(srcUri, dstUri).

Release Note

• PinotFS.copy no longer implicitly copy directory recursively, use PinotFS.copyDir instead.

[codecov-commenter]

Codecov Report

Merging #8200 (dfcaf9f) into master (e7f806b) will decrease coverage by 3.99%.
The diff coverage is 41.95%.

❗ Current head dfcaf9f differs from pull request most recent head 4eb336c. Consider uploading reports for the commit 4eb336c to get more accurate results

@@             Coverage Diff              @@
##             master    #8200      +/-   ##
============================================
- Coverage     71.34%   67.35%   -4.00%     
+ Complexity     4308     4238      -70     
============================================
  Files          1623     1226     -397     
  Lines         84365    62005   -22360     
  Branches      12657     9642    -3015     
============================================
- Hits          60192    41763   -18429     
+ Misses        20043    17279    -2764     
+ Partials       4130     2963    -1167     

Flag	Coverage Δ
integration1	?
integration2	?
unittests1	67.35% <41.95%> (-0.54%)	⬇️
unittests2	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...java/org/apache/pinot/common/config/TlsConfig.java	12.50% <ø> (ø)
...n/java/org/apache/pinot/common/utils/TlsUtils.java	0.00% <ø> (ø)
...pache/pinot/common/utils/grpc/GrpcQueryClient.java	0.00% <0.00%> (-94.74%)	⬇️
...he/pinot/core/api/ServiceAutoDiscoveryFeature.java	0.00% <0.00%> (ø)
.../org/apache/pinot/core/common/MinionConstants.java	0.00% <ø> (ø)
...rg/apache/pinot/core/transport/ListenerConfig.java	91.66% <ø> (-8.34%)	⬇️
...a/org/apache/pinot/core/transport/QueryRouter.java	74.69% <ø> (-13.26%)	⬇️
...a/org/apache/pinot/core/transport/QueryServer.java	53.19% <ø> (-21.28%)	⬇️
...rg/apache/pinot/core/transport/ServerChannels.java	76.27% <ø> (-13.56%)	⬇️
...che/pinot/core/transport/grpc/GrpcQueryServer.java	0.00% <0.00%> (-42.63%)	⬇️
... and 667 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7f806b...4eb336c. Read the comment docs.

[mcvsubbu]

Why are we making this change?

[jackjlli]

You can try the copyFromLocalDir API which is also introduced in the same previous PR, so that you don't need to change the current API in PinotFS.

pinot/pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFS.java

Line 161 in 1382d29

default void copyFromLocalDir(File srcFile, URI dstUri)

[walterddr]

You can try the copyFromLocalDir API which is also introduced in the same previous PR, so that you don't need to change the current API in PinotFS.

pinot/pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFS.java

Line 161 in 1382d29

default void copyFromLocalDir(File srcFile, URI dstUri)

Yes I notices that change in #8162, but I don't think this works for 2 reasons.

• currently S3PinotFS does a remote-to-remote recursive copy, so it will be very inefficient to download it to local and then reupload it to S3
• even if we want to go down this inefficient path, there's no recursive copyToLocalDir API

[Jackie-Jiang]

@mcvsubbu Trying to understand the the problem we are trying to solve in #8162. IIUC, we want to have an API in PinotFS that copies only file to another location, but reject the copy request if src is a directory. I'd suggest still keeping the API for directory copy because that is a very useful operation for a FS. It is caller's responsibility to call the correct API, but PinotFS should provide both functionalities. (Actually copy only file but not directory is not mandatory because caller can call isDirectory() first and reject the request when the src is a directory).

[walterddr]

Hi @Jackie-Jiang @mcvsubbu I updated the approach to

1. add a copyDir API that can copy between directories within the same PinotFS.
2. also I made a default copy implementation in PinotFS to only allow file copy and throws UOE if it is attempting to copy a directory

any additional thoughts to this approach?
this will be a behavior change so I will add release note to document the change for next release.

[Jackie-Jiang]

LGTM. @mcvsubbu Can you please take another look?

[walterddr]

Hi @mcvsubbu, any additional comments on this approach?

[mcvsubbu]

thanks for addressing all comments.

[mcvsubbu]

Thanks for clarifying the error message