Make different PinotFS concrete classes have the same behaviors by jackjlli · Pull Request #3671 · apache/pinot

jackjlli · 2019-01-10T00:06:10Z

Currently different PinotFS behaves differently.
E.g., as for listFile(URI, true) LocalPinotFS lists all the files and directories, while HadoopPinotFS only list non-dir files.
This PR:

makes sure that these two FS have the same behaviors.
follows up from [PINOT-7461] Fix segment deletion when folder under Deleted_Segment location does not exist #3589
unit tests added.

codecov-io · 2019-01-10T01:52:24Z

Codecov Report

Merging #3671 into master will increase coverage by 0.07%.
The diff coverage is 67.05%.

@@             Coverage Diff              @@
##             master    #3671      +/-   ##
============================================
+ Coverage      67.1%   67.18%   +0.07%     
- Complexity        4       44      +40     
============================================
  Files          1027     1027              
  Lines         50819    50879      +60     
  Branches       7091     7105      +14     
============================================
+ Hits          34102    34181      +79     
+ Misses        14392    14361      -31     
- Partials       2325     2337      +12

Impacted Files	Coverage Δ	Complexity Δ
...main/java/org/apache/pinot/filesystem/PinotFS.java	`100% <ø> (ø)`	`0 <0> (ø)`	⬇️
.../controller/helix/core/SegmentDeletionManager.java	`79.67% <100%> (ø)`	`0 <0> (ø)`	⬇️
...java/org/apache/pinot/filesystem/LocalPinotFS.java	`73.41% <18.75%> (-14.28%)`	`0 <0> (ø)`
...ava/org/apache/pinot/filesystem/HadoopPinotFS.java	`64.47% <77.27%> (+64.47%)`	`40 <28> (+40)`	⬆️
...a/manager/realtime/RealtimeSegmentDataManager.java	`75% <0%> (-25%)`	`0% <0%> (ø)`
...egation/function/customobject/MinMaxRangePair.java	`75.86% <0%> (-24.14%)`	`0% <0%> (ø)`
...lix/EmptyBrokerOnlineOfflineStateModelFactory.java	`86.66% <0%> (-13.34%)`	`0% <0%> (ø)`
.../apache/pinot/core/transport/DataTableHandler.java	`88% <0%> (-12%)`	`0% <0%> (ø)`
...elix/core/relocation/RealtimeSegmentRelocator.java	`70.4% <0%> (-10.21%)`	`0% <0%> (ø)`
.../apache/pinot/common/config/RealtimeTagConfig.java	`93.33% <0%> (-6.67%)`	`0% <0%> (ø)`
... and 22 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 cd2dc04...3c6279b. Read the comment docs.

pinot-filesystem/src/main/java/com/linkedin/pinot/filesystem/PinotFS.java

pinot-hadoop-filesystem/src/main/java/com/linkedin/pinot/filesystem/HadoopPinotFS.java

pinot-filesystem/src/main/java/com/linkedin/pinot/filesystem/LocalPinotFS.java

pinot-hadoop-filesystem/src/main/java/com/linkedin/pinot/filesystem/HadoopPinotFS.java

mcvsubbu

Thanks for taking this on, @jackjlli .
Can we first document the PinotFS class as we want the behavior to be?
What does copyToLocalFile do if src is a dir, dst exists (a a dir/file), etc.? I think to start with, we should define it so that we only handle the case where src is a file and dst is not there? Let us not introduce a force flag here until we iron out the interface correctly?
Same with copyFromLocalFile

mcvsubbu · 2019-02-11T19:32:16Z

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

+   * If both src and dst are files, dst will be overwritten.
+   * If src is a file and dst is a directory, src file will get copied under dst directory.
+   * If both src and dst are directories, src folder will get copied under dst directory.
+   * If src is a directory and dst is a file, operation will fail.


Clarify behavior if src is dir, dst is file, and overwrite is true. Looks like LocalFS succeeds.

mcvsubbu · 2019-02-11T21:05:47Z

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

  /**
-   * Same as move except the srcUri is not retained. For example, if x/y/z is copied to a/b/c, x/y/z will be retained
-   * and x/y/z will also be present as a/b/c
+   * Same as move except the srcUri is retained. For example, if a file x/y/z is copied to a/b/c, x/y/z will be retained


Instead of "Same as move" can you mention each case explicitly and describe the behavior? thanks.

mcvsubbu · 2019-02-11T21:06:27Z

pinot-hadoop-filesystem/src/main/java/org/apache/pinot/filesystem/HadoopPinotFS.java

-  public boolean isDirectory(URI uri) {
-    FileStatus fileStatus = new FileStatus();
-    fileStatus.setPath(new Path(uri));
+  public boolean isDirectory(URI uri) throws IOException {


can we just invoke _hadoopFS.isDirectory()

mcvsubbu · 2019-02-11T21:09:46Z

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


  /**
-   * Returns the length of the file at the provided location. Will throw exception if a directory
+   * Returns the length of the file at the provided location. Will throw exception if a directory.


Looks like this API was added to this class only for testing? Pinot code does not seem access it.

mcvsubbu · 2019-02-11T21:10:30Z

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


  /**
-   * Lists all the files at the location provided. Lists recursively.
+   * Lists all the files and directories at the location provided. Lists recursively.


Lists recursively if "recursive" is set to true.

Also, if the URI provided is not a directory, are we expected to throw exception no matter what the value of 'recursive' is? i think the localfs will not throw exception if recursive is set to false

mcvsubbu · 2019-02-11T21:28:12Z

pinot-filesystem/src/test/java/org/apache/pinot/filesystem/LocalPinotFSTest.java

    Assert.assertTrue(localPinotFS.exists(thirdTestFile.toURI()));

+    // Check that destination being directory within the source directory still works
+    File anotherFileUnderAbsoluteThreeDir = new File(newAbsoluteTempDirPath3, "anotherFile");


thanks for adding these test cases

mcvsubbu · 2019-02-11T21:30:22Z

pinot-hadoop-filesystem/src/main/java/org/apache/pinot/filesystem/HadoopPinotFS.java

+        return false;
+      } else {
+        delete(dstUri, true);
+        mkdir(dstUri);


not sure why mkdir here. shouldn't we just be doing a move and let the file system move either a file or a dir?

mcvsubbu · 2019-02-11T21:32:55Z

pinot-hadoop-filesystem/src/main/java/org/apache/pinot/filesystem/HadoopPinotFS.java

-            FileUtil.copy(_hadoopFS, sourceFiles.next().getPath(), _hadoopFS, target, true, _hadoopConf);
-        if (!succeeded) {
-          return false;
+    if (!exists(srcUri)) {


Maybe use a base class and move these there? You can add a TODO

One thing that occurs to me as I review something like this, is that for some (slow) FS, the latency of making each call may be so bad, that we are better off calling some native file system method and you get what you get. Going this route is relatively easy, and we just need to make sure that Pinot's requirements are met in all file system (i.e. if Pinot uses the case of copying from a dir to a file and that case works fine across everything, then that is alll we need).

Let us discuss a bit more before we put in too much work to unify file systems.

jackjlli · 2019-04-09T05:45:18Z

I'm closing this PR since it's been split to multiple PRs.

jackjlli requested a review from jenniferdai January 10, 2019 00:06

jackjlli force-pushed the pinotfs-copy-ensure-parent-dir-exist branch 4 times, most recently from f45a08d to 76f8e20 Compare January 10, 2019 01:18

sunithabeeram reviewed Jan 10, 2019

View reviewed changes

pinot-filesystem/src/main/java/com/linkedin/pinot/filesystem/PinotFS.java Outdated Show resolved Hide resolved

pinot-hadoop-filesystem/src/main/java/com/linkedin/pinot/filesystem/HadoopPinotFS.java Outdated Show resolved Hide resolved

sunithabeeram reviewed Jan 10, 2019

View reviewed changes

jackjlli force-pushed the pinotfs-copy-ensure-parent-dir-exist branch 3 times, most recently from 2bd4fce to 58d5a77 Compare January 15, 2019 20:56

jackjlli force-pushed the pinotfs-copy-ensure-parent-dir-exist branch from 58d5a77 to e1e8e7b Compare January 18, 2019 18:03

jackjlli force-pushed the pinotfs-copy-ensure-parent-dir-exist branch from e1e8e7b to f7c7d6b Compare February 1, 2019 23:44

Jack Li(Analytics Engineering) added 2 commits February 7, 2019 14:41

Add unit tests for copying file/dir to a non-existent file

b3ebeb1

Make different PinotFSes have the same behaviors

12adb26

jenniferdai approved these changes Feb 7, 2019

View reviewed changes

Update API doc for PinotFS

3c6279b

jackjlli force-pushed the pinotfs-copy-ensure-parent-dir-exist branch from f7c7d6b to 3c6279b Compare February 8, 2019 18:51

mcvsubbu requested changes Feb 11, 2019

View reviewed changes

mcvsubbu reviewed Feb 11, 2019

View reviewed changes

jackjlli closed this Apr 9, 2019

jackjlli deleted the pinotfs-copy-ensure-parent-dir-exist branch April 9, 2019 05:45

snyk-bot mentioned this pull request Sep 13, 2021

[Snyk] Security upgrade webpack-dev-server from 3.2.1 to 4.0.0 hypertrace/incubator-pinot#21

Closed

saxenakshitiz mentioned this pull request May 13, 2022

[Snyk] Security upgrade webpack-dev-server from 3.2.1 to 4.0.0 hypertrace/incubator-pinot#148

Closed

snyk-bot mentioned this pull request Sep 23, 2022

[Snyk] Fix for 13 vulnerabilities hypertrace/incubator-pinot#203

Closed

Conversation

jackjlli commented Jan 10, 2019

Uh oh!

codecov-io commented Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcvsubbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackjlli commented Apr 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Jan 10, 2019 •

edited

Loading