This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new 73b717b73 [util] fix result status handling
73b717b73 is described below
commit 73b717b7357b08fb0e8c0f0e9979a466eef0495e
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]>
---
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 58ad4f171..08861ff20 100644
--- a/src/kudu/server/webserver-test.cc
+++ b/src/kudu/server/webserver-test.cc
@@ -195,8 +195,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 b149447cd..8d26f32f0 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:
