AlenkaF commented on code in PR #45998: URL: https://github.com/apache/arrow/pull/45998#discussion_r3766783453
########## cpp/src/arrow/io/hdfs_test.cc: ########## @@ -1,466 +0,0 @@ -// 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. - -#include <atomic> -#include <cstdint> -#include <cstdlib> -#include <cstring> -#include <filesystem> -#include <iostream> -#include <memory> -#include <sstream> // IWYU pragma: keep -#include <string> -#include <thread> -#include <vector> - -#include <gtest/gtest.h> - -#include "arrow/buffer.h" -#include "arrow/io/hdfs.h" -#include "arrow/io/hdfs_internal.h" -#include "arrow/io/interfaces.h" -#include "arrow/status.h" -#include "arrow/testing/gtest_util.h" -#include "arrow/testing/util.h" - -namespace arrow { -namespace io { - -std::vector<uint8_t> RandomData(int64_t size) { - std::vector<uint8_t> buffer(size); - random_bytes(size, 0, buffer.data()); - return buffer; -} - -class TestHadoopFileSystem : public ::testing::Test { - public: - Status MakeScratchDir() { - if (client_->Exists(scratch_dir_)) { - RETURN_NOT_OK((client_->Delete(scratch_dir_, true))); - } - return client_->MakeDirectory(scratch_dir_); - } - - Status WriteDummyFile(const std::string& path, const uint8_t* buffer, int64_t size, - bool append = false, int buffer_size = 0, int16_t replication = 0, - int default_block_size = 0) { - std::shared_ptr<HdfsOutputStream> file; - RETURN_NOT_OK(client_->OpenWritable(path, append, buffer_size, replication, - default_block_size, &file)); - - RETURN_NOT_OK(file->Write(buffer, size)); - RETURN_NOT_OK(file->Close()); - - return Status::OK(); - } - - std::string ScratchPath(const std::string& name) { - std::stringstream ss; - ss << scratch_dir_ << "/" << name; - return ss.str(); - } - - std::string HdfsAbsPath(const std::string& relpath) { - std::stringstream ss; - ss << "hdfs://" << conf_.host << ":" << conf_.port << relpath; - return ss.str(); - } - - // Set up shared state between unit tests - void SetUp() { - internal::LibHdfsShim* driver_shim; - - client_ = nullptr; - scratch_dir_ = - (std::filesystem::temp_directory_path() / "arrow-hdfs/scratch-").string(); - int random_size = 4; - scratch_dir_.resize(scratch_dir_.size() + random_size, '%'); - random_alnum( - random_size, 0, - reinterpret_cast<uint8_t*>(&scratch_dir_[scratch_dir_.size() - random_size])); - - loaded_driver_ = false; - - Status msg = ConnectLibHdfs(&driver_shim); - if (!msg.ok()) { - if (std::getenv("ARROW_HDFS_TEST_LIBHDFS_REQUIRE")) { - FAIL() << "Loading libhdfs failed: " << msg.ToString(); - } else { - std::cout << "Loading libhdfs failed, skipping tests gracefully: " - << msg.ToString() << std::endl; - } - return; - } - - loaded_driver_ = true; - - const char* host = std::getenv("ARROW_HDFS_TEST_HOST"); - const char* port = std::getenv("ARROW_HDFS_TEST_PORT"); - const char* user = std::getenv("ARROW_HDFS_TEST_USER"); - - ASSERT_TRUE(user != nullptr) << "Set ARROW_HDFS_TEST_USER"; - - conf_.host = host == nullptr ? "localhost" : host; - conf_.user = user; - conf_.port = port == nullptr ? 20500 : atoi(port); - - ASSERT_OK(HadoopFileSystem::Connect(&conf_, &client_)); - } - - void TearDown() { - if (client_) { - if (client_->Exists(scratch_dir_)) { - ARROW_EXPECT_OK(client_->Delete(scratch_dir_, true)); - } - ARROW_EXPECT_OK(client_->Disconnect()); - } - } - - HdfsConnectionConfig conf_; - bool loaded_driver_; - - // Resources shared amongst unit tests - std::string scratch_dir_; - std::shared_ptr<HadoopFileSystem> client_; -}; - -#define SKIP_IF_NO_DRIVER() \ - if (!this->loaded_driver_) { \ - GTEST_SKIP() << "Driver not loaded, skipping"; \ - } - -TEST_F(TestHadoopFileSystem, ConnectsAgain) { - SKIP_IF_NO_DRIVER(); - - std::shared_ptr<HadoopFileSystem> client; - ASSERT_OK(HadoopFileSystem::Connect(&this->conf_, &client)); - ASSERT_OK(client->Disconnect()); -} - -TEST_F(TestHadoopFileSystem, MultipleClients) { - SKIP_IF_NO_DRIVER(); - - ASSERT_OK(this->MakeScratchDir()); - - std::shared_ptr<HadoopFileSystem> client1; - std::shared_ptr<HadoopFileSystem> client2; - ASSERT_OK(HadoopFileSystem::Connect(&this->conf_, &client1)); - ASSERT_OK(HadoopFileSystem::Connect(&this->conf_, &client2)); - ASSERT_OK(client1->Disconnect()); - - // client2 continues to function after equivalent client1 has shutdown - std::vector<HdfsPathInfo> listing; - ASSERT_OK(client2->ListDirectory(this->scratch_dir_, &listing)); - ASSERT_OK(client2->Disconnect()); -} - -TEST_F(TestHadoopFileSystem, MakeDirectory) { - SKIP_IF_NO_DRIVER(); - - std::string path = this->ScratchPath("create-directory"); - - if (this->client_->Exists(path)) { - ASSERT_OK(this->client_->Delete(path, true)); - } - - ASSERT_OK(this->client_->MakeDirectory(path)); - ASSERT_TRUE(this->client_->Exists(path)); - std::vector<HdfsPathInfo> listing; - ARROW_EXPECT_OK(this->client_->ListDirectory(path, &listing)); - ASSERT_EQ(0, listing.size()); - ARROW_EXPECT_OK(this->client_->Delete(path, true)); - ASSERT_FALSE(this->client_->Exists(path)); - ASSERT_RAISES(IOError, this->client_->ListDirectory(path, &listing)); -} - -TEST_F(TestHadoopFileSystem, GetCapacityUsed) { - SKIP_IF_NO_DRIVER(); - - // Who knows what is actually in your DFS cluster, but expect it to have - // positive used bytes and capacity - int64_t nbytes = 0; - ASSERT_OK(this->client_->GetCapacity(&nbytes)); - ASSERT_LT(0, nbytes); - - ASSERT_OK(this->client_->GetUsed(&nbytes)); - ASSERT_LT(0, nbytes); -} - -TEST_F(TestHadoopFileSystem, GetPathInfo) { - SKIP_IF_NO_DRIVER(); - - HdfsPathInfo info; - - ASSERT_OK(this->MakeScratchDir()); - - // Directory info - ASSERT_OK(this->client_->GetPathInfo(this->scratch_dir_, &info)); - ASSERT_EQ(ObjectType::DIRECTORY, info.kind); - ASSERT_EQ(this->HdfsAbsPath(this->scratch_dir_), info.name); - ASSERT_EQ(this->conf_.user, info.owner); - - // TODO(wesm): test group, other attrs - - auto path = this->ScratchPath("test-file"); - - const int size = 100; - - std::vector<uint8_t> buffer = RandomData(size); - - ASSERT_OK(this->WriteDummyFile(path, buffer.data(), size)); - ASSERT_OK(this->client_->GetPathInfo(path, &info)); - - ASSERT_EQ(ObjectType::FILE, info.kind); - ASSERT_EQ(this->HdfsAbsPath(path), info.name); - ASSERT_EQ(this->conf_.user, info.owner); - ASSERT_EQ(size, info.size); -} - -TEST_F(TestHadoopFileSystem, GetPathInfoNotExist) { - // ARROW-2919: Test that the error message is reasonable - SKIP_IF_NO_DRIVER(); - - ASSERT_OK(this->MakeScratchDir()); - auto path = this->ScratchPath("path-does-not-exist"); - - HdfsPathInfo info; - Status s = this->client_->GetPathInfo(path, &info); - ASSERT_TRUE(s.IsIOError()); - - const std::string error_message = s.ToString(); - - // Check that the file path is found in the error message - ASSERT_LT(error_message.find(path), std::string::npos); -} - -TEST_F(TestHadoopFileSystem, AppendToFile) { - SKIP_IF_NO_DRIVER(); - - ASSERT_OK(this->MakeScratchDir()); - - auto path = this->ScratchPath("test-file"); - const int size = 100; - - std::vector<uint8_t> buffer = RandomData(size); - ASSERT_OK(this->WriteDummyFile(path, buffer.data(), size)); - - // now append - ASSERT_OK(this->WriteDummyFile(path, buffer.data(), size, true)); - - HdfsPathInfo info; - ASSERT_OK(this->client_->GetPathInfo(path, &info)); - ASSERT_EQ(size * 2, info.size); -} - -TEST_F(TestHadoopFileSystem, ListDirectory) { - SKIP_IF_NO_DRIVER(); - - const int size = 100; - std::vector<uint8_t> data = RandomData(size); - - auto p1 = this->ScratchPath("test-file-1"); - auto p2 = this->ScratchPath("test-file-2"); - auto d1 = this->ScratchPath("test-dir-1"); - - ASSERT_OK(this->MakeScratchDir()); - ASSERT_OK(this->WriteDummyFile(p1, data.data(), size)); - ASSERT_OK(this->WriteDummyFile(p2, data.data(), size / 2)); - ASSERT_OK(this->client_->MakeDirectory(d1)); - - std::vector<HdfsPathInfo> listing; - ASSERT_OK(this->client_->ListDirectory(this->scratch_dir_, &listing)); - - // Do it again, appends! - ASSERT_OK(this->client_->ListDirectory(this->scratch_dir_, &listing)); - - ASSERT_EQ(6, static_cast<int>(listing.size())); - - // Argh, well, shouldn't expect the listing to be in any particular order - for (size_t i = 0; i < listing.size(); ++i) { - const HdfsPathInfo& info = listing[i]; - if (info.name == this->HdfsAbsPath(p1)) { - ASSERT_EQ(ObjectType::FILE, info.kind); - ASSERT_EQ(size, info.size); - } else if (info.name == this->HdfsAbsPath(p2)) { - ASSERT_EQ(ObjectType::FILE, info.kind); - ASSERT_EQ(size / 2, info.size); - } else if (info.name == this->HdfsAbsPath(d1)) { - ASSERT_EQ(ObjectType::DIRECTORY, info.kind); - } else { - FAIL() << "Unexpected path: " << info.name; - } - } -} - -TEST_F(TestHadoopFileSystem, ReadableMethods) { Review Comment: There has been some moving around with this test: - first it has been moved under filesystems and named `hdfs_test_io.cc`: https://github.com/apache/arrow/pull/45998/changes/b0cca50504ef28d289037a0e71fd4da055cc1c3d#diff-65fee404eb58a58c664cbbab64f066736a723a6cb5b094d94c43f8dcb076047b - then it has been renamed to `hdfs_io_test.cc`: https://github.com/apache/arrow/pull/45998/changes/caab0d078a3802f4f0fec59e99f433c2eb1a0e81 - and again renamed to `hdfs_internal_test.cc` and updated a bit when merging internal classes here: https://github.com/apache/arrow/pull/45998/changes/2bbb63097824dcb60d5a65b94109945b101bf639#diff-f4795a70b2c29c3464b6042a8a19ae801493bff7fa711e7db0d6443c53e32dea - at the end it has been removed in https://github.com/apache/arrow/pull/45998/changes/b9aab57f256606042a5ada704ceaa833b1e8961c as per this comment: https://github.com/apache/arrow/pull/45998#discussion_r3474213065 Claude suggests adding back these sections and removing the rest: - `LargeFile` (regression test) - `ThreadSafety` (concurrent use of the shared internal client) - `ConnectsAgain` / `MultipleClients` (connection lifecycle regressions) I think it is a good start. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
