raulcd commented on code in PR #45998: URL: https://github.com/apache/arrow/pull/45998#discussion_r3620959722
########## 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: Shouldn't some of these tests be moved to `arrow/filesystem/hdfs_test.cc` instead of just removing the full test file? For example we are moving `HdfsReadableFile` from the old `io/hdfs.cc` to the new `filesystem/hdfs_internal.cc` and we are not adding new tests for it (which were part of this test file). Same with `HdfsOutputStream` and other functionality. I am worried we might be reducing test coverage a tad too much as I can't see those covered deeply in the existing public API. Some of those tests should apply (with some API and include modifications) quite closely. -- 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]
