This is an automated email from the ASF dual-hosted git repository. alexey pushed a commit to branch branch-1.18.x in repository https://gitbox.apache.org/repos/asf/kudu.git
commit f2ed7f431195469b7e994f976ce6c03022ec6d82 Author: Alexey Serbin <[email protected]> AuthorDate: Wed Feb 19 20:58:16 2025 -0800 [util] fix result status handling This patch addresses sloppy result handling of functions/methods that return Status under the src/kudu/util directory and in one related test in src/kudu/server/webserver-test.cc The updated call sites are are mostly in the test code, but there are a few in src/kudu/util/thread.{cc,h} and in src/kudu/util/env_posix.cc, where the fix in the latter is the most impactful. Change-Id: Ic5dc4ebc07dca668cd33f4b83c55d4354748b937 Reviewed-on: http://gerrit.cloudera.org:8080/22514 Tested-by: Alexey Serbin <[email protected]> Reviewed-by: Marton Greber <[email protected]> (cherry picked from commit 73b717b7357b08fb0e8c0f0e9979a466eef0495e) Reviewed-on: http://gerrit.cloudera.org:8080/22544 Reviewed-by: Abhishek Chennaka <[email protected]> --- src/kudu/server/webserver-test.cc | 5 +++-- src/kudu/util/block_bloom_filter-test.cc | 2 +- src/kudu/util/char_util-test.cc | 19 ++++++++++--------- src/kudu/util/compression/compression-test.cc | 14 +++++++++----- src/kudu/util/curl_util-test.cc | 9 +++++---- src/kudu/util/curl_util.h | 6 ++---- src/kudu/util/debug-util-test.cc | 4 ++-- src/kudu/util/env-test.cc | 4 ++-- src/kudu/util/env_posix.cc | 13 ++++++++++--- src/kudu/util/file_cache-test.cc | 12 +++++++----- src/kudu/util/maintenance_manager-test.cc | 9 ++++++--- src/kudu/util/net/net_util.cc | 4 ++-- src/kudu/util/once-test.cc | 6 ++++-- src/kudu/util/pb_util-test.cc | 2 +- src/kudu/util/pb_util.cc | 2 +- src/kudu/util/thread.cc | 5 +++++ src/kudu/util/thread.h | 2 +- src/kudu/util/threadpool.cc | 3 +-- src/kudu/util/threadpool.h | 2 +- 19 files changed, 73 insertions(+), 50 deletions(-) diff --git a/src/kudu/server/webserver-test.cc b/src/kudu/server/webserver-test.cc index 07688ab63..62f43f03c 100644 --- a/src/kudu/server/webserver-test.cc +++ b/src/kudu/server/webserver-test.cc @@ -190,8 +190,9 @@ TEST_F(PasswdWebserverTest, TestPasswdPresent) { if (fips_mode) { GTEST_SKIP(); } - ASSERT_OK(curl_.set_auth(CurlAuthType::DIGEST, security::kTestAuthUsername, - security::kTestAuthPassword)); + curl_.set_auth(CurlAuthType::DIGEST, + security::kTestAuthUsername, + security::kTestAuthPassword); ASSERT_OK(curl_.FetchURL(addr_.ToString(), &buf_)); } diff --git a/src/kudu/util/block_bloom_filter-test.cc b/src/kudu/util/block_bloom_filter-test.cc index 6f5c43e18..0ab24028f 100644 --- a/src/kudu/util/block_bloom_filter-test.cc +++ b/src/kudu/util/block_bloom_filter-test.cc @@ -127,7 +127,7 @@ TEST_F(BlockBloomFilterTest, ArenaAligned) { Arena a(64); auto* allocator = a.NewObject<ArenaBlockBloomFilterBufferAllocator>(&a); auto* bf = a.NewObject<BlockBloomFilter>(allocator); - bf->Init(6, FAST_HASH, 0); + ASSERT_OK(bf->Init(6, FAST_HASH, 0)); bool key = true; Slice s(reinterpret_cast<const uint8_t*>(&key), sizeof(key)); bf->Insert(s); diff --git a/src/kudu/util/char_util-test.cc b/src/kudu/util/char_util-test.cc index b65e203c1..0aa2a4c87 100644 --- a/src/kudu/util/char_util-test.cc +++ b/src/kudu/util/char_util-test.cc @@ -19,8 +19,8 @@ #include <cstdint> #include <memory> +#include <string> -#include <glog/logging.h> #include <gtest/gtest.h> #include "kudu/util/debug/sanitizer_scopes.h" @@ -29,7 +29,7 @@ #include "kudu/util/init.h" #include "kudu/util/path_util.h" #include "kudu/util/slice.h" -#include "kudu/util/status.h" +#include "kudu/util/test_macros.h" #include "kudu/util/test_util.h" using std::unique_ptr; @@ -44,14 +44,15 @@ class CharUtilTest : public KuduTest { void SetUp() override { // UTF8Truncate uses SSE4.1 instructions so we need to make sure the CPU // running the test has these opcodes. - CHECK_OK(CheckCPUFlags()); - ReadFileToString(env_, JoinPathSegments(GetTestExecutableDirectory(), - "testdata/char_truncate_utf8.txt"), - &string_utf8_); + ASSERT_OK(CheckCPUFlags()); + ASSERT_OK(ReadFileToString(env_, JoinPathSegments(GetTestExecutableDirectory(), + "testdata/char_truncate_utf8.txt"), + &string_utf8_)); data_utf8_ = Slice(string_utf8_); - ReadFileToString(env_, JoinPathSegments(GetTestExecutableDirectory(), - "testdata/char_truncate_ascii.txt"), - &string_ascii_); + ASSERT_OK(ReadFileToString(env_, + JoinPathSegments(GetTestExecutableDirectory(), + "testdata/char_truncate_ascii.txt"), + &string_ascii_)); data_ascii_ = Slice(string_ascii_); } diff --git a/src/kudu/util/compression/compression-test.cc b/src/kudu/util/compression/compression-test.cc index df3e8ae37..f023f5303 100644 --- a/src/kudu/util/compression/compression-test.cc +++ b/src/kudu/util/compression/compression-test.cc @@ -33,6 +33,7 @@ #include "kudu/util/random.h" #include "kudu/util/random_util.h" #include "kudu/util/slice.h" +#include "kudu/util/status.h" #include "kudu/util/stopwatch.h" #include "kudu/util/test_macros.h" #include "kudu/util/test_util.h" @@ -79,7 +80,7 @@ static void TestCompressionCodec(CompressionType compression) { ASSERT_EQ(0, memcmp(ibuffer, ubuffer, kInputSize)); } -static void Benchmark(Random random, CompressionType compression) { +static Status Benchmark(Random random, CompressionType compression) { constexpr int kMaterialCount = 16; constexpr int kInputSize = 8; constexpr int kSliceCount = 1024; @@ -100,7 +101,7 @@ static void Benchmark(Random random, CompressionType compression) { // Get the specified compression codec. const CompressionCodec* codec; - GetCompressionCodec(compression, &codec); + RETURN_NOT_OK(GetCompressionCodec(compression, &codec)); // Allocate the compression buffer. size_t max_compressed = codec->MaxCompressedLength(kSliceCount * kInputSize); @@ -114,7 +115,7 @@ static void Benchmark(Random random, CompressionType compression) { Stopwatch sw; sw.start(); while (sw.elapsed().wall_seconds() < 3) { - codec->Compress(islices, cbuffer.get(), &compressed); + RETURN_NOT_OK(codec->Compress(islices, cbuffer.get(), &compressed)); total_len += kSliceCount * kInputSize; compressed_len += compressed; } @@ -131,7 +132,9 @@ static void Benchmark(Random random, CompressionType compression) { Stopwatch sw; sw.start(); while (sw.elapsed().wall_seconds() < 3) { - codec->Uncompress(Slice(cbuffer.get(), compressed), ubuffer, kSliceCount * kInputSize); + RETURN_NOT_OK(codec->Uncompress(Slice(cbuffer.get(), compressed), + ubuffer, + kSliceCount * kInputSize)); total_len += kSliceCount * kInputSize; } sw.stop(); @@ -139,6 +142,7 @@ static void Benchmark(Random random, CompressionType compression) { LOG(INFO) << CompressionType_Name(compression) << " uncompress throughput: " << mbps << " MB/sec"; } + return Status::OK(); } TEST_F(TestCompression, TestNoCompressionCodec) { @@ -156,7 +160,7 @@ TEST_F(TestCompression, TestSnappyCompressionCodec) { TEST_F(TestCompression, TestSimpleBenchmark) { Random r(SeedRandom()); for (auto type : { SNAPPY, LZ4, ZLIB }) { - NO_FATALS(Benchmark(r, type)); + ASSERT_OK(Benchmark(r, type)); } } diff --git a/src/kudu/util/curl_util-test.cc b/src/kudu/util/curl_util-test.cc index 4e15f994e..befc06a59 100644 --- a/src/kudu/util/curl_util-test.cc +++ b/src/kudu/util/curl_util-test.cc @@ -19,6 +19,7 @@ #include <functional> #include <memory> +#include <string> #include <gtest/gtest.h> @@ -44,10 +45,10 @@ TEST(CurlUtilTest, TestTimeout) { TEST(CurlUtilTest, NonSharedObjectsBetweenThreads) { const int kThreadCount = 8; std::unique_ptr<ThreadPool> pool; - ThreadPoolBuilder("curl-util-test") - .set_min_threads(kThreadCount) - .set_max_threads(kThreadCount) - .Build(&pool); + ASSERT_OK(ThreadPoolBuilder("curl-util-test") + .set_min_threads(kThreadCount) + .set_max_threads(kThreadCount) + .Build(&pool)); for (int i = 0; i < kThreadCount; i++) { ASSERT_OK(pool->Submit([&]() { diff --git a/src/kudu/util/curl_util.h b/src/kudu/util/curl_util.h index 3910d6736..436eb9794 100644 --- a/src/kudu/util/curl_util.h +++ b/src/kudu/util/curl_util.h @@ -103,12 +103,10 @@ class EasyCurl { dns_servers_ = std::move(dns_servers); } - Status set_auth(CurlAuthType auth_type, std::string username = "", std::string password = "") { - auth_type_ = std::move(auth_type); + void set_auth(CurlAuthType auth_type, std::string username = "", std::string password = "") { + auth_type_ = auth_type; username_ = std::move(username); password_ = std::move(password); - - return Status::OK(); } // Enable verbose mode for curl. This dumps debugging output to stderr, so diff --git a/src/kudu/util/debug-util-test.cc b/src/kudu/util/debug-util-test.cc index 322e17ee7..9879c60b0 100644 --- a/src/kudu/util/debug-util-test.cc +++ b/src/kudu/util/debug-util-test.cc @@ -262,7 +262,7 @@ TEST_F(DebugUtilTest, Benchmark) { volatile int prevent_optimize = 0; while (MonoTime::Now() < end_time) { StackTrace trace; - GetThreadStack(t->tid(), &trace); + ASSERT_OK(GetThreadStack(t->tid(), &trace)); if (symbolize) { prevent_optimize += trace.Symbolize().size(); } @@ -384,7 +384,7 @@ TEST_P(RaceTest, TestStackTraceRaces) { MonoTime end_time = MonoTime::Now() + MonoDelta::FromSeconds(1); while (MonoTime::Now() < end_time) { StackTrace trace; - GetThreadStack(t->tid(), &trace); + ASSERT_OK(GetThreadStack(t->tid(), &trace)); } } diff --git a/src/kudu/util/env-test.cc b/src/kudu/util/env-test.cc index 5356b4f0b..f4d6ed96f 100644 --- a/src/kudu/util/env-test.cc +++ b/src/kudu/util/env-test.cc @@ -884,8 +884,8 @@ TEST_F(TestEnv, TestGetFileModifiedTime) { // HFS has 1 second mtime granularity. AssertEventually([&] { int64_t after_time; - writer->Append(" "); - writer->Sync(); + ASSERT_OK(writer->Append(" ")); + ASSERT_OK(writer->Sync()); ASSERT_OK(env_->GetFileModifiedTime(writer->filename(), &after_time)); ASSERT_LT(initial_time, after_time); }, MonoDelta::FromSeconds(5)); diff --git a/src/kudu/util/env_posix.cc b/src/kudu/util/env_posix.cc index 1b0ddd635..853bdd8ac 100644 --- a/src/kudu/util/env_posix.cc +++ b/src/kudu/util/env_posix.cc @@ -1673,15 +1673,21 @@ class PosixEnv : public Env { const string& fname, unique_ptr<RWFile>* result) override { TRACE_EVENT1("io", "PosixEnv::NewRWFile", "path", fname); - int fd = 0; - bool encrypt = opts.is_sensitive && IsEncryptionEnabled(); + const bool encrypt = opts.is_sensitive && IsEncryptionEnabled(); uint64_t size = 0; if (opts.mode == MUST_EXIST) { RETURN_NOT_OK(GetFileSize(fname, &size)); } else if (encrypt) { - GetFileSize(fname, &size); + const auto s = GetFileSize(fname, &size); + if (PREDICT_FALSE(!s.ok() && !s.IsNotFound())) { + // The only expected non-OK status is Status::NotFound() if no file + // exists at the specified path: the use case of creating a new + // encrypted file. + return s; + } } + int fd = 0; RETURN_NOT_OK(DoOpen(fname, opts.mode, &fd)); EncryptionHeader eh; if (encrypt) { @@ -1692,6 +1698,7 @@ class PosixEnv : public Env { if (size >= kEncryptionHeaderSize) { RETURN_NOT_OK(ReadEncryptionHeader(fd, fname, *encryption_key_, &eh)); } else { + DCHECK_EQ(0, size); // overwriting non-encrypted file with encrypted one? RETURN_NOT_OK(GenerateHeader(&eh)); RETURN_NOT_OK(WriteEncryptionHeader(fd, fname, *encryption_key_, eh)); } diff --git a/src/kudu/util/file_cache-test.cc b/src/kudu/util/file_cache-test.cc index b202cb2a4..97bf9f58b 100644 --- a/src/kudu/util/file_cache-test.cc +++ b/src/kudu/util/file_cache-test.cc @@ -267,7 +267,7 @@ TYPED_TEST(FileCacheTest, TestInvalidation) { // If we invalidate it from the cache and try again, it should crash because // the existing descriptor was invalidated. this->cache_->Invalidate(kFile1); - ASSERT_DEATH({ f->Size(&size); }, "invalidated"); + ASSERT_DEATH({ ASSERT_OK(f->Size(&size)); }, "invalidated"); // But if we re-open the path again, the new descriptor should read the // new data. @@ -488,10 +488,12 @@ TEST_P(MixedFileCacheTest, TestBothFileTypes) { shared_ptr<RandomAccessFile> raf2; ASSERT_OK(cache.OpenFile<Env::MUST_EXIST>(kFile2, &raf2)); #ifndef NDEBUG - ASSERT_DEATH({ cache.OpenFile<Env::MUST_EXIST>(kFile1, &raf); }, - "!FindDescriptorUnlocked"); - ASSERT_DEATH({ cache.OpenFile<Env::MUST_EXIST>(kFile2, &rwf); }, - "!FindDescriptorUnlocked"); + ASSERT_DEATH( + { ASSERT_OK(cache.OpenFile<Env::MUST_EXIST>(kFile1, &raf)); }, + "!FindDescriptorUnlocked"); + ASSERT_DEATH( + { ASSERT_OK(cache.OpenFile<Env::MUST_EXIST>(kFile2, &rwf)); }, + "!FindDescriptorUnlocked"); #endif } diff --git a/src/kudu/util/maintenance_manager-test.cc b/src/kudu/util/maintenance_manager-test.cc index f29f19def..e44236565 100644 --- a/src/kudu/util/maintenance_manager-test.cc +++ b/src/kudu/util/maintenance_manager-test.cc @@ -48,6 +48,7 @@ #include "kudu/util/random.h" #include "kudu/util/random_util.h" #include "kudu/util/scoped_cleanup.h" +#include "kudu/util/status.h" #include "kudu/util/test_macros.h" #include "kudu/util/test_util.h" #include "kudu/util/thread.h" @@ -151,9 +152,11 @@ class TestMaintenanceOp : public MaintenanceOp { if (register_self_) { scoped_refptr<kudu::Thread> thread; // Re-register itself after 50ms. - kudu::Thread::Create("maintenance-test", "self-register", [this]() { - this->set_remaining_runs(1); - }, &thread); + CHECK_OK(kudu::Thread::Create("maintenance-test", + "self-register", + [this]() { + this->set_remaining_runs(1); + }, &thread)); } } diff --git a/src/kudu/util/net/net_util.cc b/src/kudu/util/net/net_util.cc index becd7cef2..a6a60d19b 100644 --- a/src/kudu/util/net/net_util.cc +++ b/src/kudu/util/net/net_util.cc @@ -36,7 +36,7 @@ #include <utility> #include <vector> -#include <boost/container_hash/extensions.hpp> +#include <boost/container_hash/hash.hpp> #include <gflags/gflags.h> #include <glog/logging.h> @@ -570,7 +570,7 @@ Status HostPortsFromAddrs(const vector<Sockaddr>& addrs, vector<HostPort>* hps) Status GetRandomPort(const string& address, uint16_t* port) { Sockaddr sockaddr; - sockaddr.ParseString(address, 0); + RETURN_NOT_OK(sockaddr.ParseString(address, 0)); Socket listener; RETURN_NOT_OK(listener.Init(sockaddr.family(), 0)); RETURN_NOT_OK(listener.Bind(sockaddr)); diff --git a/src/kudu/util/once-test.cc b/src/kudu/util/once-test.cc index ff8322055..348658aa7 100644 --- a/src/kudu/util/once-test.cc +++ b/src/kudu/util/once-test.cc @@ -15,14 +15,16 @@ // specific language governing permissions and limitations // under the License. +#include "kudu/util/once.h" + #include <ostream> +#include <string> #include <thread> #include <vector> #include <glog/logging.h> #include <gtest/gtest.h> -#include "kudu/util/once.h" #include "kudu/util/status.h" #include "kudu/util/test_macros.h" #include "kudu/util/test_util.h" @@ -70,7 +72,7 @@ template<class KuduOnceType> static void InitOrGetInitted(Thing<KuduOnceType>* t, int i) { if (i % 2 == 0) { LOG(INFO) << "Thread " << i << " initting"; - t->Init(); + CHECK_OK(t->Init()); } else { LOG(INFO) << "Thread " << i << " value: " << t->once_.init_succeeded(); } diff --git a/src/kudu/util/pb_util-test.cc b/src/kudu/util/pb_util-test.cc index 381d0e573..26bf859d3 100644 --- a/src/kudu/util/pb_util-test.cc +++ b/src/kudu/util/pb_util-test.cc @@ -132,7 +132,7 @@ Status TestPBUtil::NewPBCWriter(int version, RWFileOptions opts, RETURN_NOT_OK(env_->NewRWFile(opts, path_, &writer)); pb_writer->reset(new WritablePBContainerFile(std::move(writer))); if (version != kUseDefaultVersion) { - (*pb_writer)->SetVersionForTests(version); + RETURN_NOT_OK((*pb_writer)->SetVersionForTests(version)); } return Status::OK(); } diff --git a/src/kudu/util/pb_util.cc b/src/kudu/util/pb_util.cc index 564e2e00b..e3c9fb2c9 100644 --- a/src/kudu/util/pb_util.cc +++ b/src/kudu/util/pb_util.cc @@ -894,7 +894,7 @@ ReadablePBContainerFile::ReadablePBContainerFile(shared_ptr<RandomAccessFile> re } ReadablePBContainerFile::~ReadablePBContainerFile() { - Close(); + WARN_NOT_OK(Close(), "Close() failed when destroying file"); } Status ReadablePBContainerFile::Open() { diff --git a/src/kudu/util/thread.cc b/src/kudu/util/thread.cc index 0293ef1a4..d61c4921b 100644 --- a/src/kudu/util/thread.cc +++ b/src/kudu/util/thread.cc @@ -574,6 +574,11 @@ Thread::~Thread() { } } +void Thread::Join() { + WARN_NOT_OK(ThreadJoiner(this).Join(), + Substitute("$0 failed joining", ToString())); +} + string Thread::ToString() const { return Substitute("Thread $0 (name: \"$1\", category: \"$2\")", tid(), name_, category_); } diff --git a/src/kudu/util/thread.h b/src/kudu/util/thread.h index d49beb648..beecbbdbd 100644 --- a/src/kudu/util/thread.h +++ b/src/kudu/util/thread.h @@ -152,7 +152,7 @@ class Thread : public RefCountedThreadSafe<Thread> { // Blocks until this thread finishes execution. Once this method returns, the thread // will be unregistered with the ThreadMgr and will not appear in the debug UI. - void Join() { ThreadJoiner(this).Join(); } + void Join(); // A thread's OS-specific TID is assigned after it start running. However, // in order to improve the performance of thread creation, the parent diff --git a/src/kudu/util/threadpool.cc b/src/kudu/util/threadpool.cc index 6c0895f53..09e2c4ad2 100644 --- a/src/kudu/util/threadpool.cc +++ b/src/kudu/util/threadpool.cc @@ -130,12 +130,11 @@ Status SchedulerThread::Start() { thread_pool_name_, "scheduler", [this]() { this->RunLoop(); }, &thread_); } -Status SchedulerThread::Shutdown() { +void SchedulerThread::Shutdown() { if (thread_) { shutdown_.CountDown(); thread_->Join(); } - return Status::OK(); } void SchedulerThread::RunLoop() { diff --git a/src/kudu/util/threadpool.h b/src/kudu/util/threadpool.h index 2afe1c326..887594423 100644 --- a/src/kudu/util/threadpool.h +++ b/src/kudu/util/threadpool.h @@ -182,7 +182,7 @@ public: Status Start(); // Shutdown the thread and clear the pending tasks. - Status Shutdown(); + void Shutdown(); class SchedulerTask { public:
