fix(engine) #5543: expose the logger implementation through GlobalConfiguration

[robfrank]

Closes #5543

Since #5361 the logger implementation is selected with the arcadedb.log.impl system property, read in LogManager.createLogger() through a direct System.getProperty(...) call. That made it the only ArcadeDB setting outside ArcadeDB's own configuration system: it never appeared in dumpConfigAtStartup or any config-file mechanism, and because createLogger() runs from the LogManager static initializer, the value had to be set before the class was first touched, which a framework-managed application (Spring Boot) cannot always guarantee. This adds GlobalConfiguration.LOG_IMPL (arcadedb.log.impl, SCOPE.JVM, default default), whose callback installs the matching logger on the running LogManager via the extracted LogManager.createLogger(String). The system property keeps working through the very same resolution routine, so behaviour is unchanged when the setting is not touched. LogManager deliberately still reads the raw property rather than the configuration, because querying GlobalConfiguration from its static initializer would run that class's whole initialization, callbacks included, while LogManager.instance() is still null - the callback null-guards for the same reason. The issue's nit is folded in: setLogger(Logger) now has javadoc.

Test plan

• LogImplConfigurationTest written first: did not compile before the fix (cannot find symbol: variable LOG_IMPL)
• Callback neutered after the fix to prove the assertions are load-bearing: 4 of 6 tests fail, the 2 survivors being the registration/default-value checks that never reach the callback
• Setting LOG_IMPL to slf4j after startup installs Slf4jLogger; setting it back to DEFAULT (case-insensitive) installs DefaultLogger
• An unrecognized value falls back to DefaultLogger and is reported on System.err, matching the system-property behaviour from Add opt-in SLF4J facade logging (Slf4jLogger), keeping java.util.logging as default (#4276) #5361
• The setting applies through GlobalConfiguration.fromJSON(...), and appears in dumpConfiguration
• Four separate JVMs probe the class-initialization orderings that a single-JVM unit test cannot reach: property unset / slf4j / bogus, crossed with LogManager vs GlobalConfiguration loaded first. All four install the expected logger (see docs/5543-log-impl-global-configuration.md for the table)
• mvn -pl engine test: 10690 tests, 0 failures, 0 errors, 23 skipped
• mvn compile over the full reactor: green

🤖 Generated with Claude Code

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

🟢 Coverage 90.00% diff coverage · -7.66% coverage variation

Metric	Results
Coverage variation	✅ -7.66% coverage variation
Diff coverage	✅ 90.00% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (e64160a)	150064	114578	76.35%
Head commit (4ddcba4)	182065 (+32001)	125060 (+10482)	68.69% (-7.66%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#5689)	10	9	90.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Code Review - PR #5689: expose the logger implementation through GlobalConfiguration

Overall this is a clean, well-scoped change with unusually thorough documentation and a test that was demonstrably written first. The design reasoning (why LogManager keeps reading the raw system property, why no allowed set is declared given the callback runs before the allowed-values check, the re-entrancy null-guard) is sound and matches the actual code in GlobalConfiguration.setValue()/readConfiguration(). Nice work.

A few observations, one worth acting on:

1. LogManager.logger should probably be volatile (main point)

private Logger logger; (LogManager.java:47) is a non-volatile instance field, read on the hot path by every log() overload across all worker threads, and now written by setLogger().

Before this PR the field was effectively write-once via the constructor, safely published through the static final instance. This PR promotes runtime swapping to a first-class, encouraged path - a config file, fromJSON(...), or the server settings API can now call the LOG_IMPL callback -> setLogger() on an arbitrary thread while other threads are actively logging. There is no happens-before edge between that write and the reads in log(), so other threads may keep observing the stale logger reference indefinitely.

The consequence is benign (a reference swap; worst case some threads keep logging through the previous impl for a while - no crash, no corruption), but making the field volatile closes the gap correctly and is essentially free: a volatile reference read is negligible next to the formatting/IO that follows. Given the PR's whole point is "swap the logger on a running server," this seems worth doing.

2. Stored value is not normalized

The callback returns value unchanged, so setting "DEFAULT" or "SLF4J" stores the raw casing while createLogger() matches case-insensitively. So LOG_IMPL.getValue() can return "DEFAULT" while the installed logger is DefaultLogger, and an unknown "slf4" is stored verbatim even though DefaultLogger was installed. Harmless, and the test explicitly exercises the case-insensitive path, but a reader inspecting dumpConfiguration sees the un-normalized string. Optional: normalize in the callback (return the resolved canonical name) if you want the dumped value to reflect what is actually installed.

3. reset() does not re-install a logger

Correctly documented as a known limitation in the design doc, and the test's tearDown compensates by restoring originalLogger directly. Just flagging that GlobalConfiguration.LOG_IMPL.reset() leaving the swapped logger in place is a mild surprise for anyone expecting reset() to be a full undo - the doc note handles it, so this is informational only.

4. Style nit (minor)

The new entry uses a lambda value -> { ... } while every other callback in GlobalConfiguration uses new Callable<>() { ... }. The lambda is cleaner and Callable is a functional interface so it compiles fine; just noting the local convention per CLAUDE.md's "adhere to existing code." Either is defensible.

Test coverage

Good - the six tests cover registration/scope, default value, post-startup slf4j swap, case-insensitive default swap, unknown-value fallback with System.err assertion, and the fromJSON route. The four-JVM class-init ordering matrix in the doc addresses exactly what a single-JVM unit test cannot. If you add a volatile, no test change is needed.

Nothing here is blocking except point 1, which I would recommend addressing before merge.

[claude]

Review: PR #5689 - expose logger implementation through GlobalConfiguration

Solid, well-reasoned change. The core idea (make arcadedb.log.impl a first-class GlobalConfiguration entry so it flows through dumpConfigAtStartup, fromJSON, config files, and the server settings API, and can be applied after LogManager is loaded) is exactly right, and the design docs explaining the one-directional wiring and why LogManager still reads the raw property from its static initializer are genuinely helpful. TDD approach, the volatile logger fix, and the backward-compatibility guarantee are all good.

A few observations, none blocking:

1. docs/review-deferred-c4562caa.md probably shouldn't be committed

The repo's docs/ convention is NNNN-topic.md design docs keyed to an issue. review-deferred-c4562caa.md is named by a commit hash and is pure review-cycle meta-commentary (responses to bot comments). Same goes for the ## Review cycles / ### Cycle 1 section inside 5543-log-impl-global-configuration.md - that's a conversation artifact, not project documentation, and it will read as noise to future maintainers. Suggest dropping the hash-named file and the review-cycle section, keeping just the Problem/Solution/Changes/Impact content.

2. Minor: Locale inconsistency between the two normalization sites

The callback in GlobalConfiguration.LOG_IMPL lowercases with Locale.ENGLISH, while LogManager.createLogger(String) uses Locale.ROOT. No functional impact for the ASCII values slf4j/default, but since the whole point of storing the normalized spelling is that it matches what createLogger() matches on, using the same Locale in both places would remove a latent trap. (createLogger re-normalizes its input anyway, so this is cosmetic.)

3. Test isolation of process-global state

LogImplConfigurationTest mutates the process-wide LogManager singleton and System.err. This is safe today (forkCount=1, reuseForks=true, no parallel method execution in junit-platform.properties), and @AfterEach restores both, so I'm not asking for a change. Flagging only so it's on record: if JUnit parallel execution is ever enabled, these tests would need @Isolated / @ResourceLock - a concurrent test logging or writing to stderr during the swap window would make them flaky. The existing pattern of mutating GlobalConfiguration globally has the same property, so this is consistent with the codebase.

Things I checked and confirmed are fine

• The if (newValue != value) reference-comparison in setValue() correctly overwrites with the normalized string the callback returns.
• The LogManager.instance() != null guard is defensive-only in practice (createLogger() never touches GlobalConfiguration, so the callback can't actually run inside LogManager.<clinit>), but it's cheap and harmless.
• Deliberately omitting an allowed set is correct given setValue() runs the callback before the allowed-values check; a rejected value would otherwise throw after swapping the logger. This is documented.
• createLogger catches Throwable and never propagates, so the callback's exception-swallowing in setValue() can't lose a logger-swap failure - the fallback to DefaultLogger is what gets installed and returned.
• The double "unknown value" warning for a bogus value set before class load is documented and benign.

Nice work overall - the substance is clean; the only real ask is trimming the review-meta docs out of the commit.

[claude]

Review: expose the logger implementation through GlobalConfiguration (#5543)

Solid, well-scoped change. The class-initialization hazard between LogManager static initializer and GlobalConfiguration is the genuinely tricky part, and it is handled carefully: LogManager keeps reading the raw property (so it stays self-contained at static-init time), the callback null-guards LogManager.instance(), and extracting createLogger(String) keeps a single normalization/fallback routine for both entry points. The inline comments and the docs ordering table make the reasoning easy to follow, and the tests cover registration, default, post-startup swap in both directions, normalization, error fallback, and the fromJSON path. Nice TDD trail in the description too.

A few points, mostly minor / for discussion - nothing blocking:

1. Config value can diverge from the active logger. For an unrecognized value the callback stores the typo verbatim ("slf4") while the installed logger is DefaultLogger. This is intentional and documented (keep the typo visible in the dump), but it means anything introspecting LOG_IMPL.getValue() to learn the active implementation is misled. Given Expose arcadedb.log.impl through GlobalConfiguration instead of only a raw system property #5543 goal is making the setting observable through the config system, worth confirming that surfacing a rejected value as if it were the setting is the intended behavior, versus storing "default" and relying solely on the System.err warning.

2. reset() does not re-run the callback. reset() restores defValue without invoking the callback, so GlobalConfiguration.LOG_IMPL.reset() at runtime restores the config value to default but leaves whatever logger was last installed in place. The test correctly compensates by also calling setLogger(originalLogger) in tearDown, but a real caller would likely expect reset() on a side-effecting setting to reinstate the default logger. Since LOG_IMPL is unusual in having an install side effect, a one-line note in the setting Javadoc would save a future surprise.

3. Test mutates a process-wide singleton. The tests swap LogManager.instance() logger, which is JVM-global. Fine while engine tests run sequentially, but if in-JVM parallel execution is ever enabled these would race with other tests logging (and vice-versa). Consider @Isolated / @Execution(SAME_THREAD) as cheap insurance, or at least keep it in mind.

4. Nit - double normalization. The callback lower-cases/trims into impl, then createLogger(String) trims/lower-cases again. Harmless, just redundant.

5. catch (Throwable) in createLogger is broad, but the "logging init must never take the process down" rationale is documented and reasonable.

Overall a careful, well-tested fix. Points 1 and 2 are the only ones I would want an explicit answer on before merge; the rest are polish.

Reviewed with Claude Code.

[robfrank]

Answering the two points that asked for an explicit decision, and closing out the rest.

1. An unrecognized value is stored verbatim - intended. The alternative, storing default, makes the config dump agree with the installed logger but destroys the only lasting record of the typo: the System.err warning fires once at set time and is gone. A dump reading arcadedb.log.impl = slf4 next to a warning in the log is diagnosable; one reading default looks like nobody ever configured anything, which is the failure mode #5361 already fixed once by adding that warning. The setting's own description states the fallback, and LogManager.getLogger() is the accurate answer for "what is actually installed" - LOG_IMPL.getValue() answers "what was asked for", which is what every other setting means too (MAX_PAGE_RAM is the only one that rewrites, and it rewrites to a valid value it chose, not away from an invalid one the user typed).

2. reset() leaving the logger installed - noted in code, not changed. reset() runs no callback for any setting; it assigns defValue (or callbackIfNoSet). Making this one entry re-install would mean either a special case inside reset() or a contract change across all ~200 entries, neither of which belongs in this issue. Added the note you suggested as a comment on the entry (50265ee), and it is under Impact in the tracking doc.

4. Double normalization is inherent: createLogger(String) is public and also fed the raw system property, so it has to normalize its own input; the callback has to normalize separately because it stores the result. It is a trim plus lowercase of a five-character string, once per configuration change.

3. Test isolation - declining consistently with the previous cycle. @Isolated today would be an annotation guarding a mode the module does not run in, and the existing GlobalConfigurationTest / ContextConfigurationTest mutate the same kind of process-global state without one. If in-JVM parallel execution is ever switched on, that whole set moves together.

5. Agreed, and unchanged from #5361.

[claude]

Review: expose the logger implementation through GlobalConfiguration (#5689)

Overall this is a careful, well-scoped change. The design reasoning (why LogManager keeps reading the raw system property, why the callback null-guards, why an unknown value is not rewritten) is documented both in the code comments and docs/5543-*.md, which follows the existing per-issue doc convention under docs/. Test coverage is genuinely load-bearing (TDD, plus the "neutered callback" cross-check and the 4-JVM class-init matrix). No new dependency, Apache-compatible. Nice work.

Correctness / thread-safety

• Making LogManager.logger volatile is the right call now that setLogger() is promoted from an escape hatch to a runtime path written from an arbitrary thread. It matches the sibling traceContextSupplier. OK
• The class-initialization analysis holds up: in the default case (arcadedb.log.impl unset) readConfiguration() skips the callback entirely (no property, no callbackIfNoSet), so nothing installs a logger unless the value is explicitly set. I traced the reverse ordering too - neither DefaultLogger/Slf4jLogger construction nor LogManager.<clinit> touches GlobalConfiguration, so there is no cross-thread <clinit> cycle, and the instance() == null guard covers the defensive re-entrant case. OK
• setValue() overwrites value with the callback return via reference inequality (newValue != value); since the callback returns a freshly built impl string, the normalized spelling is what gets stored. Verified against dumpConfiguration()/toJSON(). OK

Minor observations (non-blocking)

1. Double normalization. The callback trims+lowercases into impl (GlobalConfiguration.java:156-158) and then createLogger(impl) trims+lowercases again (LogManager.java:162). Harmless and cheap, but the callback normalization exists solely to control the stored spelling while createLogger re-derives it for matching - worth a one-line note that the redundancy is intentional. Not important enough to change.
2. volatile on the log hot path. Every log() overload now does a volatile read of logger. This is the correct trade for visibility, and a relaxed volatile read is effectively free on the common architectures, so given CLAUDE.md's performance mantra I would just flag it as a conscious correctness-over-micro-optimization choice rather than a concern.
3. Process-global side effect from a setting. Setting LOG_IMPL swaps the JVM-wide singleton logger for every embedded instance/database in the JVM. That is semantically consistent with SCOPE.JVM, so it is fine - just calling it out for readers.
4. reset() asymmetry (leaves the swapped logger installed) is already documented in the code comment and Impact section, and the test @AfterEach restores explicitly. Reasonable to defer; agree it is out of scope to change the reset() contract for one entry.

Test coverage

Strong. Registration/scope/dump, default value, slf4j-after-startup, default-after-startup (case-insensitive), whitespace/case normalization, unknown-value fallback with System.err capture, and fromJSON. One gap worth noting: there is no direct assertion that the setting surfaces via the server settings API (GetServerHandler) - the PR body claims it but only asserts dumpConfiguration. Low priority since it follows automatically from SCOPE.JVM.

Net: LGTM. The minor items above are informational, not blockers.

[claude]

Review: expose arcadedb.log.impl through GlobalConfiguration (#5689)

Overall this is a clean, well-reasoned change. The design correctly threads the one-directional wiring (system property read directly at static-init time, GlobalConfiguration.LOG_IMPL driving setLogger() afterwards) and the PR description / docs/5543-*.md do an excellent job pre-empting most reviewer questions. Tests are written test-first, cover the meaningful paths (post-startup swap, case/whitespace normalization, unknown-value fallback + System.err reporting, fromJSON), and are correctly restored in @AfterEach. Verified against the source: the createLogger(String) extraction, the volatile promotion, setValue()s callback-before-allowed ordering, and readConfiguration()s setValue path all line up with the reasoning given.

A few observations, none blocking:

Correctness / behavior

• The volatile logger is the right call and well justified: setLogger() is now a genuine runtime write from an arbitrary thread while log() overloads read on every thread, so the reference must be safely published. Matches the sibling traceContextSupplier.
• Double installation + object churn when arcadedb.log.impl is set as a system property: on the ordering where GlobalConfiguration initializes with the property set, readConfiguration -> setValue -> callback constructs a second Slf4jLogger and re-installs it, on top of the one LogManagers static init already built. Harmless (and you document the bogus-prints-twice case), but a small side-effect worth being aware of. Not worth an equality short-circuit.
• reset() leaving the swapped logger installed is documented in both the code comment and impact section; acceptable given the alternative is a per-entry special case or a contract change across all settings.

Security

• Because LOG_IMPL is SCOPE.JVM, it is surfaced by GetServerHandler (which filters only != DATABASE), so on a running server the logger implementation becomes settable/visible through the settings API. Intended, and not an injection vector (createLogger only maps to the two fixed classes and never reflects on the input), but it is a new runtime-mutable, server-wide side effect reachable by whoever can write JVM-scope settings. Worth a line in the changelog/security notes so operators know a settings-API caller can now swap the log backend on a live server. No code change needed.

Minor / nits

• com.arcadedb.utility.Callable is not annotated @FunctionalInterface; the lambda still compiles (single abstract method), just noting it is not enforced.
• The unknown-value warning on System.err from within a config callback is consistent with the pre-existing Add opt-in SLF4J facade logging (Slf4jLogger), keeping java.util.logging as default (#4276) #5361 behavior and intentional (the logger may not be trustworthy at that point). Fine as is.

Test coverage
Good coverage of the single-JVM behavior. The class-initialization-ordering matrix is inherently untestable in-process (LogManager is already loaded in the test JVM); the documented four-JVM manual probe is the pragmatic substitute. Not asserting that the setting reaches the server settings API is reasonable since it follows purely from SCOPE.JVM.

LGTM. Careful piece of work with unusually thorough documentation of the tradeoffs.

[codecov]

Codecov Report

❌ Patch coverage is 60.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.69%. Comparing base (e64160a) to head (4ddcba4).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...rc/main/java/com/arcadedb/GlobalConfiguration.java	62.50%	1 Missing and 2 partials ⚠️
...ine/src/main/java/com/arcadedb/log/LogManager.java	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5689      +/-   ##
============================================
- Coverage     67.53%   66.69%   -0.84%     
- Complexity        0     1118    +1118     
============================================
  Files          1770     1770              
  Lines        150064   150073       +9     
  Branches      31807    31808       +1     
============================================
- Hits         101342   100094    -1248     
- Misses        35527    36946    +1419     
+ Partials      13195    13033     -162     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.