wgtmac commented on code in PR #725:
URL: https://github.com/apache/iceberg-cpp/pull/725#discussion_r3475138791
##########
src/iceberg/logging/logger.h:
##########
@@ -371,3 +371,179 @@ void Log(Logger& logger, LogLevel level,
}
} // namespace iceberg
+
+// ---------------------------------------------------------------------------
+// Logging macros.
+//
+// Every macro takes a std::format string followed by its arguments. The
+// rendered line depends on the active backend (see cerr_logger.h for the
+// std::cerr layout, or the spdlog pattern); the examples below show the call
+// site and, for the default CerrLogger, the line it produces.
+//
+// ICEBERG_LOG_TRACE("entering scan for {}", table);
+// 2026-06-16T10:59:41.186Z trace [12345] table_scan.cc:88] entering scan
for db.t
+// ICEBERG_LOG_DEBUG("cache miss key={}", key);
+// 2026-06-16T10:59:41.186Z debug [12345] cache.cc:42] cache miss
key=manifest-7
+// ICEBERG_LOG_INFO("loaded {} manifests in {} ms", n, ms);
+// 2026-06-16T10:59:41.186Z info [12345] table_scan.cc:91] loaded 5
manifests in 12 ms
+// ICEBERG_LOG_WARN("retry {} after {}", attempt, err);
+// 2026-06-16T10:59:41.186Z warn [12345] io.cc:51] retry 2 after timeout
+// ICEBERG_LOG_ERROR("commit failed: {}", status);
+// 2026-06-16T10:59:41.186Z error [12345] txn.cc:77] commit failed:
conflict
+// ICEBERG_LOG_CRITICAL("metadata unreadable at {}", path);
+// 2026-06-16T10:59:41.186Z critical [12345] meta.cc:30] metadata
unreadable at
+// s3://b/m.json
+// ICEBERG_LOG_FATAL("unrecoverable: {}", reason); // emits, flushes, then
+// std::abort()
+// 2026-06-16T10:59:41.186Z fatal [12345] boot.cc:19] unrecoverable: bad
config
+//
+// Less common forms:
+// ICEBERG_LOG(level, "level chosen at runtime: {}", x); // runtime
severity
+// ICEBERG_LOG_TO(logger, level, "to an explicit logger {}", y);
+// ICEBERG_LOG_RUNTIME_FMT(level, fmt_string, args...); // non-literal
format
+//
+// With ICEBERG_LOG_SHORT_MACROS defined, bare aliases (LOG_INFO, ...) are also
+// available. A format string is mandatory; zero extra args is fine
+// (ICEBERG_LOG_INFO("done")).
+// ---------------------------------------------------------------------------
+
+/// \brief Compile-time severity floor: statements below this level are removed
+/// entirely from the build (their format call sites and source_location
literals
+/// are never emitted). Defaults to keeping everything. ICEBERG_LOG_FATAL is
never
+/// gated by this floor -- its abort is always compiled in.
+#ifndef ICEBERG_LOG_ACTIVE_LEVEL
+# define ICEBERG_LOG_ACTIVE_LEVEL ::iceberg::LogLevel::kTrace
+#endif
+
+// Internal: fixed-severity emit with compile-time floor then the authoritative
+// Logger::ShouldLog (the single source of truth for runtime filtering), with
+// formatting only on the taken path, never throwing.
+#define ICEBERG_INTERNAL_LOG(level_, FMT_, ...)
\
+ do {
\
+ if constexpr ((level_) >= ICEBERG_LOG_ACTIVE_LEVEL) {
\
+ const auto& _ib_logger = ::iceberg::internal::CurrentLogger();
\
+ if (_ib_logger && _ib_logger->ShouldLog(level_)) {
\
+ try {
\
+ ::iceberg::internal::Emit(*_ib_logger, (level_),
\
+ ::std::source_location::current(),
\
+ ::std::format(FMT_ __VA_OPT__(, )
__VA_ARGS__)); \
Review Comment:
**Performance/Heap Allocation:** `std::format` returns a `std::string`. If
the formatted message exceeds the Small String Optimization (SSO) limit, this
will trigger a heap allocation before the logger even processes it. For a
high-performance system like Iceberg, this could become a bottleneck on hot
paths. Consider using a stack/thread-local buffer (like `fmt::memory_buffer` in
`fmtlib` or `spdlog`) to achieve zero-allocation formatting.
##########
src/iceberg/logging/logger.h:
##########
@@ -371,3 +371,179 @@ void Log(Logger& logger, LogLevel level,
}
} // namespace iceberg
+
+// ---------------------------------------------------------------------------
+// Logging macros.
+//
+// Every macro takes a std::format string followed by its arguments. The
+// rendered line depends on the active backend (see cerr_logger.h for the
+// std::cerr layout, or the spdlog pattern); the examples below show the call
+// site and, for the default CerrLogger, the line it produces.
+//
+// ICEBERG_LOG_TRACE("entering scan for {}", table);
+// 2026-06-16T10:59:41.186Z trace [12345] table_scan.cc:88] entering scan
for db.t
+// ICEBERG_LOG_DEBUG("cache miss key={}", key);
+// 2026-06-16T10:59:41.186Z debug [12345] cache.cc:42] cache miss
key=manifest-7
+// ICEBERG_LOG_INFO("loaded {} manifests in {} ms", n, ms);
+// 2026-06-16T10:59:41.186Z info [12345] table_scan.cc:91] loaded 5
manifests in 12 ms
+// ICEBERG_LOG_WARN("retry {} after {}", attempt, err);
+// 2026-06-16T10:59:41.186Z warn [12345] io.cc:51] retry 2 after timeout
+// ICEBERG_LOG_ERROR("commit failed: {}", status);
+// 2026-06-16T10:59:41.186Z error [12345] txn.cc:77] commit failed:
conflict
+// ICEBERG_LOG_CRITICAL("metadata unreadable at {}", path);
+// 2026-06-16T10:59:41.186Z critical [12345] meta.cc:30] metadata
unreadable at
+// s3://b/m.json
+// ICEBERG_LOG_FATAL("unrecoverable: {}", reason); // emits, flushes, then
+// std::abort()
+// 2026-06-16T10:59:41.186Z fatal [12345] boot.cc:19] unrecoverable: bad
config
+//
+// Less common forms:
+// ICEBERG_LOG(level, "level chosen at runtime: {}", x); // runtime
severity
+// ICEBERG_LOG_TO(logger, level, "to an explicit logger {}", y);
+// ICEBERG_LOG_RUNTIME_FMT(level, fmt_string, args...); // non-literal
format
+//
+// With ICEBERG_LOG_SHORT_MACROS defined, bare aliases (LOG_INFO, ...) are also
+// available. A format string is mandatory; zero extra args is fine
+// (ICEBERG_LOG_INFO("done")).
+// ---------------------------------------------------------------------------
+
+/// \brief Compile-time severity floor: statements below this level are removed
+/// entirely from the build (their format call sites and source_location
literals
+/// are never emitted). Defaults to keeping everything. ICEBERG_LOG_FATAL is
never
+/// gated by this floor -- its abort is always compiled in.
+#ifndef ICEBERG_LOG_ACTIVE_LEVEL
+# define ICEBERG_LOG_ACTIVE_LEVEL ::iceberg::LogLevel::kTrace
+#endif
+
+// Internal: fixed-severity emit with compile-time floor then the authoritative
+// Logger::ShouldLog (the single source of truth for runtime filtering), with
+// formatting only on the taken path, never throwing.
+#define ICEBERG_INTERNAL_LOG(level_, FMT_, ...)
\
+ do {
\
+ if constexpr ((level_) >= ICEBERG_LOG_ACTIVE_LEVEL) {
\
+ const auto& _ib_logger = ::iceberg::internal::CurrentLogger();
\
+ if (_ib_logger && _ib_logger->ShouldLog(level_)) {
\
+ try {
\
+ ::iceberg::internal::Emit(*_ib_logger, (level_),
\
+ ::std::source_location::current(),
\
+ ::std::format(FMT_ __VA_OPT__(, )
__VA_ARGS__)); \
+ } catch (...) {
\
Review Comment:
**Exception Handling Scope:** `catch (...)` is very broad and will swallow
all exceptions, including `std::bad_alloc` (OOM) or thread cancellation
exceptions (e.g., POSIX `abi::__forced_unwind`). Since `std::format` typically
throws `std::format_error`, it is safer to catch `const std::format_error&` or
`const std::exception&` so fatal system exceptions can propagate.
##########
src/iceberg/logging/logger.h:
##########
@@ -371,3 +371,179 @@ void Log(Logger& logger, LogLevel level,
}
} // namespace iceberg
+
+// ---------------------------------------------------------------------------
+// Logging macros.
+//
+// Every macro takes a std::format string followed by its arguments. The
+// rendered line depends on the active backend (see cerr_logger.h for the
+// std::cerr layout, or the spdlog pattern); the examples below show the call
+// site and, for the default CerrLogger, the line it produces.
+//
+// ICEBERG_LOG_TRACE("entering scan for {}", table);
+// 2026-06-16T10:59:41.186Z trace [12345] table_scan.cc:88] entering scan
for db.t
+// ICEBERG_LOG_DEBUG("cache miss key={}", key);
+// 2026-06-16T10:59:41.186Z debug [12345] cache.cc:42] cache miss
key=manifest-7
+// ICEBERG_LOG_INFO("loaded {} manifests in {} ms", n, ms);
+// 2026-06-16T10:59:41.186Z info [12345] table_scan.cc:91] loaded 5
manifests in 12 ms
+// ICEBERG_LOG_WARN("retry {} after {}", attempt, err);
+// 2026-06-16T10:59:41.186Z warn [12345] io.cc:51] retry 2 after timeout
+// ICEBERG_LOG_ERROR("commit failed: {}", status);
+// 2026-06-16T10:59:41.186Z error [12345] txn.cc:77] commit failed:
conflict
+// ICEBERG_LOG_CRITICAL("metadata unreadable at {}", path);
+// 2026-06-16T10:59:41.186Z critical [12345] meta.cc:30] metadata
unreadable at
+// s3://b/m.json
+// ICEBERG_LOG_FATAL("unrecoverable: {}", reason); // emits, flushes, then
+// std::abort()
+// 2026-06-16T10:59:41.186Z fatal [12345] boot.cc:19] unrecoverable: bad
config
+//
+// Less common forms:
+// ICEBERG_LOG(level, "level chosen at runtime: {}", x); // runtime
severity
+// ICEBERG_LOG_TO(logger, level, "to an explicit logger {}", y);
+// ICEBERG_LOG_RUNTIME_FMT(level, fmt_string, args...); // non-literal
format
+//
+// With ICEBERG_LOG_SHORT_MACROS defined, bare aliases (LOG_INFO, ...) are also
+// available. A format string is mandatory; zero extra args is fine
+// (ICEBERG_LOG_INFO("done")).
+// ---------------------------------------------------------------------------
+
+/// \brief Compile-time severity floor: statements below this level are removed
+/// entirely from the build (their format call sites and source_location
literals
+/// are never emitted). Defaults to keeping everything. ICEBERG_LOG_FATAL is
never
+/// gated by this floor -- its abort is always compiled in.
+#ifndef ICEBERG_LOG_ACTIVE_LEVEL
+# define ICEBERG_LOG_ACTIVE_LEVEL ::iceberg::LogLevel::kTrace
+#endif
+
+// Internal: fixed-severity emit with compile-time floor then the authoritative
+// Logger::ShouldLog (the single source of truth for runtime filtering), with
+// formatting only on the taken path, never throwing.
+#define ICEBERG_INTERNAL_LOG(level_, FMT_, ...)
\
Review Comment:
**Binary Bloat:** Expanding this entire block (including `if`, `try-catch`,
and `ShouldLog`) at every log call site can significantly bloat the binary size
and reduce instruction cache hit rates. A common optimization is to use type
erasure (e.g., `std::make_format_args`) and push the actual `try-catch`
execution into a non-inline function in the `.cc` file.
##########
src/iceberg/logging/logger.h:
##########
@@ -371,3 +371,179 @@ void Log(Logger& logger, LogLevel level,
}
} // namespace iceberg
+
+// ---------------------------------------------------------------------------
+// Logging macros.
+//
+// Every macro takes a std::format string followed by its arguments. The
+// rendered line depends on the active backend (see cerr_logger.h for the
+// std::cerr layout, or the spdlog pattern); the examples below show the call
+// site and, for the default CerrLogger, the line it produces.
+//
+// ICEBERG_LOG_TRACE("entering scan for {}", table);
+// 2026-06-16T10:59:41.186Z trace [12345] table_scan.cc:88] entering scan
for db.t
+// ICEBERG_LOG_DEBUG("cache miss key={}", key);
+// 2026-06-16T10:59:41.186Z debug [12345] cache.cc:42] cache miss
key=manifest-7
+// ICEBERG_LOG_INFO("loaded {} manifests in {} ms", n, ms);
+// 2026-06-16T10:59:41.186Z info [12345] table_scan.cc:91] loaded 5
manifests in 12 ms
+// ICEBERG_LOG_WARN("retry {} after {}", attempt, err);
+// 2026-06-16T10:59:41.186Z warn [12345] io.cc:51] retry 2 after timeout
+// ICEBERG_LOG_ERROR("commit failed: {}", status);
+// 2026-06-16T10:59:41.186Z error [12345] txn.cc:77] commit failed:
conflict
+// ICEBERG_LOG_CRITICAL("metadata unreadable at {}", path);
+// 2026-06-16T10:59:41.186Z critical [12345] meta.cc:30] metadata
unreadable at
+// s3://b/m.json
+// ICEBERG_LOG_FATAL("unrecoverable: {}", reason); // emits, flushes, then
+// std::abort()
+// 2026-06-16T10:59:41.186Z fatal [12345] boot.cc:19] unrecoverable: bad
config
+//
+// Less common forms:
+// ICEBERG_LOG(level, "level chosen at runtime: {}", x); // runtime
severity
+// ICEBERG_LOG_TO(logger, level, "to an explicit logger {}", y);
+// ICEBERG_LOG_RUNTIME_FMT(level, fmt_string, args...); // non-literal
format
+//
+// With ICEBERG_LOG_SHORT_MACROS defined, bare aliases (LOG_INFO, ...) are also
+// available. A format string is mandatory; zero extra args is fine
+// (ICEBERG_LOG_INFO("done")).
+// ---------------------------------------------------------------------------
+
+/// \brief Compile-time severity floor: statements below this level are removed
+/// entirely from the build (their format call sites and source_location
literals
+/// are never emitted). Defaults to keeping everything. ICEBERG_LOG_FATAL is
never
+/// gated by this floor -- its abort is always compiled in.
+#ifndef ICEBERG_LOG_ACTIVE_LEVEL
+# define ICEBERG_LOG_ACTIVE_LEVEL ::iceberg::LogLevel::kTrace
+#endif
+
+// Internal: fixed-severity emit with compile-time floor then the authoritative
+// Logger::ShouldLog (the single source of truth for runtime filtering), with
+// formatting only on the taken path, never throwing.
+#define ICEBERG_INTERNAL_LOG(level_, FMT_, ...)
\
+ do {
\
+ if constexpr ((level_) >= ICEBERG_LOG_ACTIVE_LEVEL) {
\
+ const auto& _ib_logger = ::iceberg::internal::CurrentLogger();
\
+ if (_ib_logger && _ib_logger->ShouldLog(level_)) {
\
+ try {
\
+ ::iceberg::internal::Emit(*_ib_logger, (level_),
\
+ ::std::source_location::current(),
\
+ ::std::format(FMT_ __VA_OPT__(, )
__VA_ARGS__)); \
+ } catch (...) {
\
+ ::iceberg::internal::EmitFormatError(*_ib_logger, (level_),
\
+
::std::source_location::current()); \
+ }
\
+ }
\
+ }
\
+ } while (0)
+
+#define ICEBERG_LOG_TRACE(...) \
+ ICEBERG_INTERNAL_LOG(::iceberg::LogLevel::kTrace, __VA_ARGS__)
+#define ICEBERG_LOG_DEBUG(...) \
+ ICEBERG_INTERNAL_LOG(::iceberg::LogLevel::kDebug, __VA_ARGS__)
+#define ICEBERG_LOG_INFO(...) \
+ ICEBERG_INTERNAL_LOG(::iceberg::LogLevel::kInfo, __VA_ARGS__)
+#define ICEBERG_LOG_WARN(...) \
+ ICEBERG_INTERNAL_LOG(::iceberg::LogLevel::kWarn, __VA_ARGS__)
+#define ICEBERG_LOG_ERROR(...) \
+ ICEBERG_INTERNAL_LOG(::iceberg::LogLevel::kError, __VA_ARGS__)
+#define ICEBERG_LOG_CRITICAL(...) \
+ ICEBERG_INTERNAL_LOG(::iceberg::LogLevel::kCritical, __VA_ARGS__)
+
+// FATAL: emit if enabled (never compile-stripped), then ALWAYS flush + abort.
+// Acquires the default logger ONCE and uses the same instance for emit and
flush
+// so a concurrent SetDefaultLogger cannot flush a different logger than it
emitted to.
+#define ICEBERG_LOG_FATAL(FMT_, ...)
\
+ do {
\
+ auto _ib_logger = ::iceberg::GetDefaultLogger();
\
+ if (_ib_logger && _ib_logger->ShouldLog(::iceberg::LogLevel::kFatal)) {
\
+ try {
\
+ ::iceberg::internal::Emit(*_ib_logger, ::iceberg::LogLevel::kFatal,
\
+ ::std::source_location::current(),
\
+ ::std::format(FMT_ __VA_OPT__(, )
__VA_ARGS__)); \
+ } catch (...) {
\
+ ::iceberg::internal::EmitFormatError(*_ib_logger,
::iceberg::LogLevel::kFatal, \
+
::std::source_location::current()); \
+ }
\
+ }
\
+ if (_ib_logger) _ib_logger->Flush();
\
+ ::std::abort();
\
Review Comment:
**Extensibility:** Hardcoding `std::abort()` removes the ability for the
host application (like a Java JNI or Python process embedding Iceberg) to
handle fatal errors gracefully (e.g., flushing resources or printing a stack
trace). Consider allowing the user to register a custom `FatalHandler` callback
before falling back to `abort()`.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
