You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by jl...@apache.org on 2019/02/26 21:59:37 UTC
[incubator-pinot] branch fix-move-files-when-it-is-a-new-table
updated: Address PR comments
This is an automated email from the ASF dual-hosted git repository.
jlli pushed a commit to branch fix-move-files-when-it-is-a-new-table
in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git
The following commit(s) were added to refs/heads/fix-move-files-when-it-is-a-new-table by this push:
new 0fb94b5 Address PR comments
0fb94b5 is described below
commit 0fb94b50564a54cfe368dd81a2e1d9f98c35e568
Author: jackjlli <jl...@linkedin.com>
AuthorDate: Tue Feb 26 13:59:16 2019 -0800
Address PR comments
---
.../org/apache/pinot/filesystem/AzurePinotFS.java | 5 -----
.../java/org/apache/pinot/filesystem/PinotFS.java | 21 ++++++++++++++-------
.../apache/pinot/filesystem/LocalPinotFSTest.java | 9 +++++++++
3 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/pinot-azure-filesystem/src/main/java/org/apache/pinot/filesystem/AzurePinotFS.java b/pinot-azure-filesystem/src/main/java/org/apache/pinot/filesystem/AzurePinotFS.java
index f8422c0..0f07f67 100644
--- a/pinot-azure-filesystem/src/main/java/org/apache/pinot/filesystem/AzurePinotFS.java
+++ b/pinot-azure-filesystem/src/main/java/org/apache/pinot/filesystem/AzurePinotFS.java
@@ -98,11 +98,6 @@ public class AzurePinotFS extends PinotFS {
}
@Override
- /**
- * Note: One corner scenario for this method in AzureFS is if the destination is a non-empty directory,
- * then the call fails rather than overwrite the directory. Thus, it's best to specify
- * the full paths of both src and dst when using this method in AzurePinotFS.
- */
protected boolean doMove(URI srcUri, URI dstUri)
throws IOException {
return _adlStoreClient.rename(srcUri.getPath(), dstUri.getPath());
diff --git a/pinot-filesystem/src/main/java/org/apache/pinot/filesystem/PinotFS.java b/pinot-filesystem/src/main/java/org/apache/pinot/filesystem/PinotFS.java
index f79208c..c4c7e1e 100644
--- a/pinot-filesystem/src/main/java/org/apache/pinot/filesystem/PinotFS.java
+++ b/pinot-filesystem/src/main/java/org/apache/pinot/filesystem/PinotFS.java
@@ -29,8 +29,14 @@ import org.slf4j.LoggerFactory;
/**
- * The PinotFS is intended to be a thin wrapper on top of different filesystems. This interface is intended for internal
- * Pinot use only. This class will be implemented for each pluggable storage type.
+ * PinotFS is a restricted FS API that exposes functionality that is required for Pinot to use
+ * different FS implementations. The restrictions are in place due to 2 driving factors:
+ * 1. Prevent unexpected performance hit when a broader API is implemented - especially, we would like
+ * to reduce calls to remote filesystems that might be needed for a broader API,
+ * but not necessarily required by Pinot (see the documentation for move() method below).
+ * 2. Provide an interface that is simple to be implemented across different FS types.
+ * The contract that developers have to adhere to will be simpler.
+ * Please read the method level docs carefully to note the exceptions while using the APIs.
*/
public abstract class PinotFS implements Closeable {
private static final Logger LOGGER = LoggerFactory.getLogger(PinotFS.class);
@@ -66,8 +72,9 @@ public abstract class PinotFS implements Closeable {
* Note: In Pinot we recommend the full paths of both src and dst be specified.
* For example, if a file /a/b/c is moved to a file /x/y/z, in the case of overwrite, the directory /a/b still exists,
* but will not contain the file 'c'. Instead, /x/y/z will contain the contents of 'c'.
- * If a directory /a/b/ is renamed to another directory /x/y/, the directory /x/y/ will contains the content of /a/b/.
- * If a directory /a/b/ is moved under the directory /x/y/, the dst needs to be specify as /x/y/b/.
+ * If src is a directory /a/b which contains two files /a/b/c and /a/b/d, and the dst is /x/y, the result would be
+ * that the directory /a/b under /a gets removed and dst directory contains two files which is /x/y/c and /x/y/d.
+ * If src is a directory /a/b needs to be moved under another directory /x/y, please specify the dst to /x/y/b.
* @param srcUri URI of the original file
* @param dstUri URI of the final file location
* @param overwrite true if we want to overwrite the dstURI, false otherwise
@@ -107,9 +114,9 @@ public abstract class PinotFS implements Closeable {
* that haven't been created, this method will create all the necessary parent directories.
* Note: In Pinot we recommend the full paths of both src and dst be specified.
* For example, if a file /a/b/c is copied to a file /x/y/z, in the case of overwrite, the directory /a/b still exists,
- * but will not contain the file 'c'. Instead, /x/y/z will contain the contents of 'c'.
- * If a directory /a/b/ is copied to another directory /x/y/, the directory /x/y/ will contains the content of /a/b/.
- * If a directory /a/b/ is copied under the directory /x/y/, the dst needs to be specify as /x/y/b/.
+ * containing the file 'c'. The dst file /x/y/z will contain the contents of 'c'.
+ * If a directory /a/b is copied to another directory /x/y, the directory /x/y will contain the content of /a/b.
+ * If a directory /a/b is copied under the directory /x/y, the dst needs to be specify as /x/y/b.
* @param srcUri URI of the original file
* @param dstUri URI of the final file location
* @return true if copy is successful
diff --git a/pinot-filesystem/src/test/java/org/apache/pinot/filesystem/LocalPinotFSTest.java b/pinot-filesystem/src/test/java/org/apache/pinot/filesystem/LocalPinotFSTest.java
index 27dcaee..5f1c595 100644
--- a/pinot-filesystem/src/test/java/org/apache/pinot/filesystem/LocalPinotFSTest.java
+++ b/pinot-filesystem/src/test/java/org/apache/pinot/filesystem/LocalPinotFSTest.java
@@ -210,6 +210,15 @@ public class LocalPinotFSTest {
localPinotFS.copy(firstTempDir.toURI(), secondTempDir.toURI());
Assert.assertEquals(localPinotFS.listFiles(secondTempDir.toURI(), true).length, 1);
+ // Copying directory with files under another directory.
+ File firstTempDirUnderSecondTempDir = new File(secondTempDir, firstTempDir.getName());
+ localPinotFS.copy(firstTempDir.toURI(), firstTempDirUnderSecondTempDir.toURI());
+ Assert.assertTrue(localPinotFS.exists(firstTempDirUnderSecondTempDir.toURI()));
+ // There're two files/directories under secondTempDir.
+ Assert.assertEquals(localPinotFS.listFiles(secondTempDir.toURI(), false).length, 2);
+ // The file under src directory also got copied under dst directory.
+ Assert.assertEquals(localPinotFS.listFiles(firstTempDirUnderSecondTempDir.toURI(), true).length, 1);
+
// len of dir = exception
try {
localPinotFS.length(firstTempDir.toURI());
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org