This is an automated email from the ASF dual-hosted git repository.
lxy-9602 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/paimon-cpp.git
The following commit(s) were added to refs/heads/main by this push:
new dc404189 fix(logging): format variadic arguments before forwarding to
glog (#457)
dc404189 is described below
commit dc40418978c04b3a7746eb2a43de77f0132e4fc9
Author: Nicholas Jiang <[email protected]>
AuthorDate: Fri Oct 23 14:14:15 2026 +0700
fix(logging): format variadic arguments before forwarding to glog (#457)
---
src/paimon/common/logging/logging.cpp | 11 +++-
src/paimon/common/logging/logging_test.cpp | 95 +++++++++++++++++-------------
2 files changed, 63 insertions(+), 43 deletions(-)
diff --git a/src/paimon/common/logging/logging.cpp
b/src/paimon/common/logging/logging.cpp
index 252e7a52..c91d8fc9 100644
--- a/src/paimon/common/logging/logging.cpp
+++ b/src/paimon/common/logging/logging.cpp
@@ -21,6 +21,7 @@
#include <cerrno>
#include <cstdarg>
+#include <cstdio>
#include <cstdlib>
#include <mutex>
#include <optional>
@@ -78,15 +79,23 @@ class GlogAdaptor : public Logger {
public:
void LogV(PaimonLogLevel level, const char* fname, int lineno, const char*
function,
const char* fmt, ...) override {
+ // RawLog__ is variadic and cannot take a va_list, so format the
message first.
+ char message[kMessageBufferSize] = {};
va_list args;
va_start(args, fmt);
- google::RawLog__(ToGlogLevel(level), fname, lineno, fmt, args);
+ std::vsnprintf(message, sizeof(message), fmt, args);
va_end(args);
+ google::RawLog__(ToGlogLevel(level), fname, lineno, "%s", message);
}
bool IsLevelEnabled(PaimonLogLevel level) const override {
return true;
}
+
+ private:
+ // Stays below glog's 3000-byte raw log buffer, so long messages are
truncated
+ // instead of being replaced by glog's "Message was too long" error.
+ static constexpr size_t kMessageBufferSize = 2048;
};
std::unique_ptr<Logger> Logger::GetLogger(const std::string& path) {
diff --git a/src/paimon/common/logging/logging_test.cpp
b/src/paimon/common/logging/logging_test.cpp
index d7fe9832..3c391a4b 100644
--- a/src/paimon/common/logging/logging_test.cpp
+++ b/src/paimon/common/logging/logging_test.cpp
@@ -19,7 +19,8 @@
#include "paimon/logging.h"
#include <atomic>
-#include <cstdarg>
+#include <cstdlib>
+#include <functional>
#include <future>
#include <iostream>
#include <memory>
@@ -30,7 +31,6 @@
#include "glog/log_severity.h"
#include "glog/logging.h"
-#include "glog/raw_logging.h"
#include "paimon/common/executor/future.h"
#include "paimon/executor.h"
#include "paimon/fs/file_system.h"
@@ -40,35 +40,31 @@ namespace {
std::atomic<int> g_creator_calls{0};
-// A Logger that forwards to glog exactly like the built-in adaptor. It is
used to
-// restore behavior-preserving logging after exercising the custom-creator
path,
-// because the registry cannot be reset to "unset" through the public API.
-class GlogForwardingLogger : public Logger {
- public:
- void LogV(PaimonLogLevel level, const char* fname, int lineno, const char*
/*function*/,
- const char* fmt, ...) override {
- va_list args;
- va_start(args, fmt);
- google::RawLog__(ToGlog(level), fname, lineno, fmt, args);
- va_end(args);
- }
+std::string CaptureInfoLogs(const std::function<void()>& log) {
+ const int32_t prev_stderrthreshold = FLAGS_stderrthreshold;
+ FLAGS_stderrthreshold = google::GLOG_INFO;
+ ::testing::internal::CaptureStderr();
+ log();
+ std::string output = ::testing::internal::GetCapturedStderr();
+ FLAGS_stderrthreshold = prev_stderrthreshold;
+ return output;
+}
- bool IsLevelEnabled(PaimonLogLevel /*level*/) const override {
- return true;
- }
+void RegisterCustomLoggerCreatorAndExit() {
+ auto default_logger = Logger::GetLogger("default_path");
+ const Logger* expected_logger = default_logger.get();
+ g_creator_calls.store(0);
+ Logger::RegisterLogger([&default_logger](const std::string&) ->
std::unique_ptr<Logger> {
+ g_creator_calls.fetch_add(1);
+ return std::move(default_logger);
+ });
- private:
- static google::LogSeverity ToGlog(PaimonLogLevel level) {
- switch (level) {
- case PAIMON_LOG_LEVEL_WARN:
- return google::GLOG_WARNING;
- case PAIMON_LOG_LEVEL_ERROR:
- return google::GLOG_ERROR;
- default:
- return google::GLOG_INFO;
- }
- }
-};
+ auto logger = Logger::GetLogger("custom_path");
+ const bool used_creator = logger && logger.get() == expected_logger &&
+ g_creator_calls.load() == 1 &&
+ logger->IsLevelEnabled(PAIMON_LOG_LEVEL_INFO);
+ std::_Exit(used_creator ? 0 : 1);
+}
} // namespace
TEST(LoggerTest, TestMultiThreadGetLogger) {
@@ -99,6 +95,30 @@ TEST(LoggerTest, TestLogAllSeverities) {
logger->LogV(PAIMON_LOG_LEVEL_MAX, __FILE__, __LINE__, __FUNCTION__, "max
severity");
}
+TEST(LoggerTest, TestLogFormatsArguments) {
+ auto logger = Logger::GetLogger("format_test");
+ ASSERT_TRUE(logger);
+
+ const std::string output = CaptureInfoLogs(
+ [&]() { PAIMON_LOG_INFO(logger, "name=%s count=%d raw=%s", "paimon",
42, "100%d"); });
+ ASSERT_NE(output.find("name=paimon count=42 raw=100%d"),
std::string::npos) << output;
+}
+
+TEST(LoggerTest, TestLogTruncatesLongMessage) {
+ auto logger = Logger::GetLogger("truncate_test");
+ ASSERT_TRUE(logger);
+ constexpr size_t kMaxMessageLength = 2047;
+ const std::string fitting_message = "[" + std::string(kMaxMessageLength -
2, 'a') + "]";
+ const std::string long_message = "[" + std::string(4096, 'b');
+
+ const std::string output = CaptureInfoLogs([&]() {
+ PAIMON_LOG_INFO(logger, "%s", fitting_message.c_str());
+ PAIMON_LOG_INFO(logger, "%s", long_message.c_str());
+ });
+ ASSERT_NE(output.find(fitting_message + "\n"), std::string::npos);
+ ASSERT_NE(output.find(long_message.substr(0, kMaxMessageLength) + "\n"),
std::string::npos);
+}
+
// Demonstrates that glog's normal LOG() path actually writes a log file to
disk.
// Note: Paimon's Logger/GlogAdaptor uses google::RawLog__, which only goes to
// stderr and never touches disk, so this test drives glog's file sink
directly.
@@ -166,19 +186,10 @@ TEST(LoggerTest, TestGlogWritesLogFileToDisk) {
FLAGS_minloglevel = prev_minloglevel;
}
-// Keep this test last: it installs a process-wide logger creator that cannot
be
-// unset, so it must not run before tests that rely on the default logger.
+// A registered logger creator cannot be unset, so register it in a separate
process
+// to keep the default logger for other tests regardless of test order.
TEST(LoggerTest, TestRegisterCustomLoggerCreator) {
- g_creator_calls.store(0);
- Logger::RegisterLogger([](const std::string& /*path*/) ->
std::unique_ptr<Logger> {
- g_creator_calls.fetch_add(1);
- return std::make_unique<GlogForwardingLogger>();
- });
-
- auto logger = Logger::GetLogger("custom_path");
- ASSERT_TRUE(logger);
- // GetLogger must have gone through the registered creator branch.
- ASSERT_EQ(1, g_creator_calls.load());
- ASSERT_TRUE(logger->IsLevelEnabled(PAIMON_LOG_LEVEL_INFO));
+ ::testing::FLAGS_gtest_death_test_style = "threadsafe";
+ ASSERT_EXIT(RegisterCustomLoggerCreatorAndExit(),
::testing::ExitedWithCode(0), "");
}
} // namespace paimon::test
