Resolve comments

facebookincubator · Sep 29, 2024 · 98e4c5c · 98e4c5c
1 parent 3608224
commit 98e4c5c
Show file tree

Hide file tree

Showing 6 changed files with 21 additions and 13 deletions.
diff --git a/velox/connectors/hive/storage_adapters/hdfs/HdfsFileSystem.cpp b/velox/connectors/hive/storage_adapters/hdfs/HdfsFileSystem.cpp
@@ -18,6 +18,11 @@
 #include "velox/common/config/Config.h"
 #include "velox/connectors/hive/storage_adapters/hdfs/HdfsReadFile.h"
 #include "velox/connectors/hive/storage_adapters/hdfs/HdfsWriteFile.h"
+#include "velox/external/hdfs/ArrowHdfsInternal.h"
+
+namespace filesystems::arrow::io::internal {
+class LibHdfsShim;
+}
 
 namespace facebook::velox::filesystems {
 std::string_view HdfsFileSystem::kScheme("hdfs://");

diff --git a/velox/connectors/hive/storage_adapters/hdfs/HdfsFileSystem.h b/velox/connectors/hive/storage_adapters/hdfs/HdfsFileSystem.h
@@ -14,12 +14,9 @@
  * limitations under the License.
  */
 #include "velox/common/file/FileSystems.h"
-#include "velox/external/hdfs/ArrowHdfsInternal.h"
 
 namespace facebook::velox::filesystems {
 
-using filesystems::arrow::io::internal::LibHdfsShim;
-
 struct HdfsServiceEndpoint {
   HdfsServiceEndpoint(const std::string& hdfsHost, const std::string& hdfsPort)
       : host(hdfsHost), port(hdfsPort) {}

diff --git a/velox/connectors/hive/storage_adapters/hdfs/tests/CMakeLists.txt b/velox/connectors/hive/storage_adapters/hdfs/tests/CMakeLists.txt
@@ -52,6 +52,3 @@ target_link_libraries(
 
 target_compile_options(velox_hdfs_insert_test
                        PRIVATE -Wno-deprecated-declarations)
-
-set_tests_properties(velox_hdfs_insert_test PROPERTIES DEPENDS
-                                                       velox_hdfs_file_test)
diff --git a/velox/connectors/hive/storage_adapters/hdfs/tests/HdfsMiniCluster.cpp b/velox/connectors/hive/storage_adapters/hdfs/tests/HdfsMiniCluster.cpp
@@ -28,7 +28,7 @@ void HdfsMiniCluster::start() {
         noMapReduceOption,
         formatNameNodeOption,
         httpPortOption,
-        httpPort,
+        httpPort_,
         nameNodePortOption,
         nameNodePort,
         configurationOption,
@@ -60,7 +60,9 @@ bool HdfsMiniCluster::isRunning() {
 }
 
 // requires hadoop executable to be on the PATH
-HdfsMiniCluster::HdfsMiniCluster() {
+HdfsMiniCluster::HdfsMiniCluster(std::string httpPort, std::string nameNodePort)
+    : httpPort_(httpPort), nameNodePort_(nameNodePort) {
+  filesystemUrl_ = "hdfs://localhost:" + nameNodePort_;
   env_ = (boost::process::environment)boost::this_process::environment();
   env_["PATH"] = env_["PATH"].to_string() + hadoopSearchPath;
   auto path = env_["PATH"].to_vector();
@@ -82,7 +84,7 @@ void HdfsMiniCluster::addFile(std::string source, std::string destination) {
       exePath_,
       filesystemCommand,
       filesystemUrlOption,
-      filesystemUrl,
+      filesystemUrl_,
       filePutOption,
       source,
       destination);

diff --git a/velox/connectors/hive/storage_adapters/hdfs/tests/HdfsMiniCluster.h b/velox/connectors/hive/storage_adapters/hdfs/tests/HdfsMiniCluster.h
@@ -31,19 +31,19 @@ static const std::string miniclusterCommand{"minicluster"};
 static const std::string noMapReduceOption{"-nomr"};
 static const std::string formatNameNodeOption{"-format"};
 static const std::string httpPortOption{"-nnhttpport"};
-static const std::string httpPort{"7676"};
 static const std::string nameNodePortOption{"-nnport"};
 static const std::string nameNodePort{"7878"};
 static const std::string configurationOption{"-D"};
 static const std::string turnOffPermissions{"dfs.permissions=false"};
 static const std::string filesystemCommand{"fs"};
 static const std::string filesystemUrlOption{"-fs"};
-static const std::string filesystemUrl{"hdfs://localhost:" + nameNodePort};
 static const std::string filePutOption{"-put"};
 
 class HdfsMiniCluster {
  public:
-  HdfsMiniCluster();
+  HdfsMiniCluster(
+      std::string httpPort = "7676",
+      std::string nameNodePort = "7878");
 
   void start();
 
@@ -60,5 +60,8 @@ class HdfsMiniCluster {
   std::unique_ptr<::boost::process::child> serverProcess_;
   boost::filesystem::path exePath_;
   boost::process::environment env_;
+  std::string httpPort_;
+  std::string nameNodePort_;
+  std::string filesystemUrl_;
 };
 } // namespace facebook::velox::filesystems::test
diff --git a/velox/connectors/hive/storage_adapters/hdfs/tests/InsertIntoHdfsTest.cpp b/velox/connectors/hive/storage_adapters/hdfs/tests/InsertIntoHdfsTest.cpp
@@ -22,6 +22,7 @@
 #include "velox/exec/tests/utils/AssertQueryBuilder.h"
 #include "velox/exec/tests/utils/HiveConnectorTestBase.h"
 #include "velox/exec/tests/utils/PlanBuilder.h"
+#include "velox/exec/tests/utils/PortUtil.h"
 
 using namespace facebook::velox;
 using namespace facebook::velox::core;
@@ -37,7 +38,10 @@ class InsertIntoHdfsTest : public HiveConnectorTestBase {
   void SetUp() override {
     HiveConnectorTestBase::SetUp();
     if (miniCluster == nullptr) {
-      miniCluster = std::make_shared<filesystems::test::HdfsMiniCluster>();
+      auto httpPort = getFreePort();
+      auto nameNodePort = getFreePort();
+      miniCluster = std::make_shared<filesystems::test::HdfsMiniCluster>(
+          std::to_string(httpPort), std::to_string(nameNodePort));
       miniCluster->start();
     }
   }