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 <[email protected]>
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: [email protected]
For additional commands, e-mail: [email protected]
