You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by ja...@apache.org on 2021/07/09 01:06:27 UTC
[incubator-pinot] branch master updated: Switch to using
'authority' instead of 'host' when building parent URI in PinotFS::move
(#7037)
This is an automated email from the ASF dual-hosted git repository.
jackie pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git
The following commit(s) were added to refs/heads/master by this push:
new f0675af Switch to using 'authority' instead of 'host' when building parent URI in PinotFS::move (#7037)
f0675af is described below
commit f0675af0dd092ac35da5a32969ed56e9d3524908
Author: Caner Balci <ca...@gmail.com>
AuthorDate: Thu Jul 8 18:06:02 2021 -0700
Switch to using 'authority' instead of 'host' when building parent URI in PinotFS::move (#7037)
Switch to using 'authority' instead of 'host' when building parent URI in PinotFS:move method.
This fixes a bug when a FileSystem implementation uses URI with ports.
---
.../org/apache/pinot/spi/filesystem/PinotFS.java | 18 +--
.../apache/pinot/spi/filesystem/PinotFSTest.java | 180 +++++++++++++++++++++
2 files changed, 181 insertions(+), 17 deletions(-)
diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFS.java b/pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFS.java
index 84e5f5e..06e2936 100644
--- a/pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFS.java
+++ b/pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFS.java
@@ -108,23 +108,7 @@ public abstract class PinotFS implements Closeable, Serializable {
// ensures the parent path of dst exists.
try {
Path parentPath = Paths.get(dstUri.getPath()).getParent();
- /**
- * Use authority instead of host if the value contains "_": uri.getHost() will be null
- * This is related to https://bugs.java.com/bugdatabase/view_bug.do?bug_id=6587184
- * @see java.net.URI.Parser#parseHostname
- */
- String host = dstUri.getHost();
- if (host == null) {
- host = dstUri.getAuthority();
- }
- /**
- * Use the 5 element constructor in case the host parameter contains "_",
- * to ensure getHost() does not return null.
- * This is possible when using some implementations of PinotFS, for example:
- *
- * @see org.apache.pinot.plugin.filesystem.GcsPinotFS
- */
- URI parentUri = new URI(dstUri.getScheme(), host, parentPath.toString(), null, null);
+ URI parentUri = new URI(dstUri.getScheme(), dstUri.getAuthority(), parentPath.toString(), null, null);
mkdir(parentUri);
} catch (URISyntaxException e) {
throw new IOException(e);
diff --git a/pinot-spi/src/test/java/org/apache/pinot/spi/filesystem/PinotFSTest.java b/pinot-spi/src/test/java/org/apache/pinot/spi/filesystem/PinotFSTest.java
new file mode 100644
index 0000000..cdf659a
--- /dev/null
+++ b/pinot-spi/src/test/java/org/apache/pinot/spi/filesystem/PinotFSTest.java
@@ -0,0 +1,180 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.filesystem;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.URI;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+
+public class PinotFSTest {
+ public String srcForMoveFile = "myfs://root/file1";
+ public String dstForMoveFile = "myfs://root/someDir/file1";
+ public String srcForMoveFileWithPort = "myfs://myhost1:1234/root/file1";
+ public String dstForMoveFileWithPort = "myfs://myhost2:1234/someDir/file1";
+
+ @Test
+ public void testMoveFileUriGeneration() throws Exception {
+ MockRemoteFS fs = new MockRemoteFS();
+ fs.init(null);
+ fs.move(new URI(srcForMoveFile), new URI(dstForMoveFile), false);
+
+ Assert.assertEquals(fs.mkdirCalls, 1,"should call mkdir once");
+ Assert.assertEquals(fs.mkdirArgs.get(0).toString(), "myfs://root/someDir", "should create correct parent");
+
+ Assert.assertEquals(fs.doMoveCalls, 1, "doMove should be called once");
+ Map<String, URI> callArgs = fs.doMoveArgs.get(0);
+ Assert.assertEquals(callArgs.get("srcUri").toString(), srcForMoveFile, "should keep correct src");
+ Assert.assertEquals(callArgs.get("dstUri").toString(), dstForMoveFile, "should keep correct dst");
+ }
+
+ @Test
+ public void testMoveFileUriGenerationWithPort() throws Exception {
+ MockRemoteFS fs = new MockRemoteFS();
+ fs.init(null);
+ fs.move(new URI(srcForMoveFileWithPort), new URI(dstForMoveFileWithPort), false);
+
+ Assert.assertEquals(fs.mkdirCalls, 1,"should call mkdir once");
+ Assert.assertEquals(fs.mkdirArgs.get(0).toString(), "myfs://myhost2:1234/someDir", "should create correct parent");
+
+ Assert.assertEquals(fs.doMoveCalls, 1, "doMove should be called once");
+ Map<String, URI> callArgs = fs.doMoveArgs.get(0);
+ Assert.assertEquals(callArgs.get("srcUri").toString(), srcForMoveFileWithPort, "should keep correct src");
+ Assert.assertEquals(callArgs.get("dstUri").toString(), dstForMoveFileWithPort, "should keep correct dst");
+ }
+
+ /**
+ * MockRemoteFS implementation used to test behavior of the Abstract class PinotFS
+ */
+ private class MockRemoteFS extends PinotFS {
+ public int doMoveCalls;
+ public List<Map<String, URI>> doMoveArgs;
+
+ public int mkdirCalls;
+ public List<URI> mkdirArgs;
+
+ @Override
+ public void init(PinotConfiguration config) {
+ doMoveCalls = 0;
+ doMoveArgs = new ArrayList<>();
+
+ mkdirCalls = 0;
+ mkdirArgs = new ArrayList<>();
+ }
+
+ @Override
+ public boolean mkdir(URI uri)
+ throws IOException {
+ mkdirArgs.add(uri);
+ mkdirCalls++;
+ return true;
+ }
+
+ @Override
+ public boolean delete(URI segmentUri, boolean forceDelete)
+ throws IOException {
+ return false;
+ }
+
+ @Override
+ public boolean doMove(URI srcUri, URI dstUri)
+ throws IOException {
+ HashMap<String, URI> call = new HashMap<>();
+ call.put("srcUri", srcUri);
+ call.put("dstUri", dstUri);
+ doMoveArgs.add(call);
+ doMoveCalls++;
+ return true;
+ }
+
+ @Override
+ public boolean copy(URI srcUri, URI dstUri)
+ throws IOException {
+ return false;
+ }
+
+ @Override
+ public boolean exists(URI fileUri)
+ throws IOException {
+
+ if (fileUri.toString() == srcForMoveFile ||
+ fileUri.toString() == srcForMoveFileWithPort) {
+ return true;
+ }
+
+ return false;
+ }
+
+ @Override
+ public long length(URI fileUri)
+ throws IOException {
+ return 0;
+ }
+
+ @Override
+ public String[] listFiles(URI fileUri, boolean recursive)
+ throws IOException {
+ return new String[0];
+ }
+
+ @Override
+ public void copyToLocalFile(URI srcUri, File dstFile)
+ throws Exception {
+
+ }
+
+ @Override
+ public void copyFromLocalFile(File srcFile, URI dstUri)
+ throws Exception {
+
+ }
+
+ @Override
+ public boolean isDirectory(URI uri)
+ throws IOException {
+ return false;
+ }
+
+ @Override
+ public long lastModified(URI uri)
+ throws IOException {
+ return 0;
+ }
+
+ @Override
+ public boolean touch(URI uri)
+ throws IOException {
+ return false;
+ }
+
+ @Override
+ public InputStream open(URI uri)
+ throws IOException {
+ return null;
+ }
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org