Up to standards ✅

_{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.}

Code Review

Overall: This is a clean, well-scoped feature PR. The implementation is minimal, the refactoring is correct, and the test coverage is adequate. A few things worth addressing.

What the PR Does

Adds arcadedb.ha.raftStorageDirectory (GlobalConfiguration.HA_RAFT_STORAGE_DIRECTORY) so the parent directory of raft-storage-<peerId> can be decoupled from the server root path. Motivated by Kubernetes readOnlyRootFilesystem deployments where the entire server root cannot be writable.

Issues

1. GlobalConfiguration vs ContextConfiguration split in test cleanup (potential maintenance trap)

RaftHAServer.getRaftStorageDir() reads from the instance-level ContextConfiguration, but BaseRaftHATest.deleteDatabaseFolders() reads from the static GlobalConfiguration.HA_RAFT_STORAGE_DIRECTORY:

// BaseRaftHATest.java - reads GLOBAL config
String raftDir = GlobalConfiguration.HA_RAFT_STORAGE_DIRECTORY.getValueAsString();

// RaftHAServer.java - reads per-server ContextConfiguration
String raftDir = configuration.getValueAsString(GlobalConfiguration.HA_RAFT_STORAGE_DIRECTORY);

This works now only because RaftStorageDirectoryIT overrides deleteDatabaseFolders() to manually delete the custom dir. But any future test that calls config.setValue(HA_RAFT_STORAGE_DIRECTORY, ...) in onServerConfiguration() without also overriding deleteDatabaseFolders() will silently leave stale raft dirs behind. Consider adding a comment to BaseRaftHATest.deleteDatabaseFolders() warning that subclasses setting HA_RAFT_STORAGE_DIRECTORY via per-server config must override deleteDatabaseFolders() too.

2. Unnecessary null check in getRaftStorageDir()

String raftDir = configuration.getValueAsString(GlobalConfiguration.HA_RAFT_STORAGE_DIRECTORY);
if (raftDir == null || raftDir.isEmpty())  // null branch is dead - default is ""

The config entry has a String.class, "" default, so getValueAsString() will never return null for this key. The null guard is harmless but misleading. Consider replacing isEmpty() with isBlank(), which also catches whitespace-only values that would otherwise produce a broken path like " /raft-storage-...".

3. Missing @Tag("IntegrationTest") on RaftStorageDirectoryIT

Several peer tests in the same package carry @Tag("IntegrationTest") (e.g. RaftHAServerIT, RaftReplicationIT, RaftHAComprehensiveIT). RaftStorageDirectoryIT starts a full 2-node cluster and fits that profile. Note that RaftBootstrapDoesNotEngageOnRestartIT (which the PR cites as a regression test) also lacks the tag, so this may be intentional - worth double-checking against CI filter config.

Minor Suggestions

• Relative paths in production: The config description recommends Set to an absolute path (e.g. /var/lib/arcadedb/raft) but the setting silently accepts relative paths. A startup-time warning log when !Paths.get(raftDir).isAbsolute() would help operators catch misconfigurations before they wonder why raft data landed in the working directory.

• docs/4446-configurable-raft-storage-path.md: Consistent with the project convention used by other issues. No concerns.

What's Good

• The getRaftStorageDir() helper cleanly eliminates three identical path-construction expressions - good refactoring.
• Backward compatibility is preserved with a zero-migration empty-string default.
• The new IT test is correctly scoped: verifies the feature, avoids testing Ratis internals, and cleans up after itself.
• The GlobalConfiguration entry placement (between HA_RAFT_PERSIST_STORAGE and HA_SNAPSHOT_THRESHOLD) is logical.

Code Review - PR #4446: Configurable Raft Storage Directory

Overall this is a well-scoped, clean feature. The use case (Kubernetes readOnlyRootFilesystem) is clear, backward compatibility is preserved, and the implementation is concise. A few items need attention before merge.

Bug: Duplicate Javadoc block in BaseRaftHATest.java

The original Javadoc was not removed when the enhanced version was added. Both blocks are present back-to-back (lines 84-112 in the diff), which will confuse readers and may produce Javadoc warnings:

  /**
   * Extends the base cleanup to also remove Raft storage directories.
   * ... (original, shorter block — should be deleted)
   */
  /**
   * Extends the base cleanup to also remove Raft storage directories.
   * ... (new block with NOTE — keep this one)
   */
  @Override
  protected void deleteDatabaseFolders() {

Remove the first block.

docs/review-deferred-75cb6b3.md should not be committed

This file is a meta-artifact from a previous AI review cycle, not project documentation. It tracks review suggestions that were intentionally deferred and names claude[bot] as the source. It has no value to future contributors browsing the docs/ directory and will confuse them. Please remove it before merging.

docs/4446-configurable-raft-storage-path.md is legitimate design documentation and is fine to keep.

Test coverage gap - negative assertion missing

RaftStorageDirectoryIT only asserts the positive case (custom dir exists). It does not assert that no raft-storage-* directories were created in the default server root. Without the negative check, a regression where Raft uses both paths would still pass the test:

// Add this negative assertion alongside the positive one
final File defaultDir = new File(
    GlobalConfiguration.SERVER_ROOT_PATH.getValueAsString(),
    "raft-storage-" + peerIdForIndex(i));
assertThat(defaultDir)
    .as("raft-storage for server %d must NOT exist under default server root when raftStorageDirectory is set", i)
    .doesNotExist();

Missing @Tag("IntegrationTest") on RaftStorageDirectoryIT

Peer classes that start a full multi-node cluster (RaftHAServerIT, RaftReplicationIT, RaftHAComprehensiveIT) carry @Tag("IntegrationTest"). RaftStorageDirectoryIT starts a 2-node cluster and fits that profile. The deferred-review doc notes this was left open deliberately, but the annotation is cheap and keeps CI filter behaviour consistent.

Design note: deleteDatabaseFolders() reads global, not per-server config

This is documented in the new Javadoc, which is good. However, it creates a subtle footgun: subclasses that configure HA_RAFT_STORAGE_DIRECTORY via onServerConfiguration() (per-server ContextConfiguration) silently get the wrong cleanup path unless they override deleteDatabaseFolders(). The comment documents the contract, but future test authors may miss it.

Consider whether deleteDatabaseFolders() should instead accept the raft dir as a parameter passed from a hook, or whether subclasses should store the configured path in a field. For now the documented override pattern is acceptable, but it is worth flagging.

What looks good

• getRaftStorageDir() helper is clean - single source of truth, replaces 3 identical string concatenations, uses File(File, String) constructor correctly.
• Backward compatibility: empty string default means zero migration cost.
• null || isBlank() guard in both getRaftStorageDir() and deleteDatabaseFolders() handles edge cases.
• Config description is clear about the Kubernetes use case and includes a concrete example path.
• The null guard on SERVER_ROOT_PATH in deleteDatabaseFolders() is correct defensive coding.

Summary

/gemini review

test comment - please ignore

Code Review

Overview

This PR adds a configurable arcadedb.ha.raftStorageDirectory setting so Raft storage sub-folders can be placed outside the server root, primarily enabling Kubernetes deployments with readOnlyRootFilesystem: true. The implementation is clean and minimal, the default is backward-compatible, and a regression test is included.

Strengths

• Correct encapsulation: The private getRaftStorageDir() helper centralises the path logic and eliminates three copies of the same string concatenation.
• Zero migration cost: Empty default preserves the existing layout with no operator action required on upgrade.
• Proper null/blank guard in both production code and test cleanup.
• Balanced test assertions: RaftStorageDirectoryIT checks both that the dirs appear under the custom path and that they do NOT appear under the default server root.
• Javadoc comment on the cleanup-architecture limitation in BaseRaftHATest is useful for future test authors.

Issues and Suggestions

Issue 1 - Missing @tag(slow) on RaftStorageDirectoryIT

The new test starts a 2-node Raft cluster, which takes multiple seconds. Similar tests in the same package carry this annotation: RaftLeaderCrashWithExternalPropertyIT, RaftPriorityRejoinIT, RaftLargeSchemaReplicationIT all use @tag(slow). RaftStorageDirectoryIT should follow the same convention. Note: RaftBootstrapDoesNotEngageOnRestartIT also appears to be missing this tag and is similarly slow - worth tagging while in the area.

Issue 2 - Fragile base-class cleanup for per-server config overrides

BaseRaftHATest.deleteDatabaseFolders() reads GlobalConfiguration.HA_RAFT_STORAGE_DIRECTORY from the global static config, but RaftStorageDirectoryIT sets the value via per-server ContextConfiguration in onServerConfiguration(). The two do not share state, so the base cleanup silently becomes a no-op for the custom directory - which is why the override in RaftStorageDirectoryIT is necessary. The Javadoc comment calls this out, which is the right mitigation. A lighter alternative would be to store the resolved raft dir in a protected field on BaseRaftHATest that subclasses can set, so the base cleanup can always do the right thing without requiring a full override. The current approach is correct for this PR - this is a suggestion, not a blocker.

Issue 3 - No warning for relative paths in production

When raftDir is a relative path (e.g. ./target/custom-raft-storage as used in the test), it resolves relative to the JVM working directory rather than the server root. This is fine for tests but could surprise operators in production. A log warning when the configured path is not absolute would help with diagnosability. Not blocking.

Minor nits

• The leading ./ in CUSTOM_RAFT_DIR = ./target/custom-raft-storage is redundant in Java file paths but harmless.
• The doc file (docs/4446-configurable-raft-storage-path.md) follows the established project convention for issue design docs.
• GlobalConfiguration entry placement between HA_RAFT_PERSIST_STORAGE and HA_SNAPSHOT_THRESHOLD is logical.

Summary

The implementation is solid and solves a real operational pain point cleanly. The main actionable item before merge is adding @tag(slow) to RaftStorageDirectoryIT to keep CI build times consistent with the rest of the HA test suite. The other points are suggestions.

Code Review

Overview

This PR adds arcadedb.ha.raftStorageDirectory to decouple Raft storage from the server root path - a clean, focused change that addresses a real operational pain point for Kubernetes deployments with readOnlyRootFilesystem: true. The implementation is backward-compatible (empty default) and the logic is well-encapsulated.

Positives

• getRaftStorageDir() helper: Clean extraction of the path logic into a single private method. All 3 call sites consistently updated. Using new File(parent, child) constructor instead of string concatenation is the right approach for platform portability.
• Null/blank guards: Consistent null || isBlank() pattern used in both the helper and the test cleanup - correctly handles both a null return from config and a whitespace-only value.
• Test assertions: RaftStorageDirectoryIT checks both the positive case (dir exists under custom path) and the negative case (dir does NOT exist under server root), which is the right coverage for a path-redirection feature.
• @Tag("slow") at class level: Correct per the project conventions.
• BaseRaftHATest javadoc warning: The note about reading from global static config vs. per-server ContextConfiguration is exactly the kind of non-obvious constraint that belongs in a comment.

Issues

Minor - RaftBootstrapLateNewerJoinerIT still hardcodes the path

RaftBootstrapLateNewerJoinerIT.java:131 constructs the raft storage path directly from GlobalConfiguration.SERVER_ROOT_PATH without going through getRaftStorageDir() or respecting HA_RAFT_STORAGE_DIRECTORY:

final File raftStorage = new File(rootPath + File.separator + "raft-storage-" + peerIdForIndex(LATE_JOINER_INDEX));

This won't fail today because that test never sets a custom directory, but it is a silent inconsistency - if someone later sets HA_RAFT_STORAGE_DIRECTORY globally, this test's cleanup will silently miss the directory. Worth updating to use GlobalConfiguration.HA_RAFT_STORAGE_DIRECTORY the same way BaseRaftHATest.deleteDatabaseFolders() does, or using a shared helper.

Nit - super.deleteDatabaseFolders() call in RaftStorageDirectoryIT does redundant work

RaftStorageDirectoryIT.deleteDatabaseFolders() calls super.deleteDatabaseFolders() (which walks SERVER_ROOT_PATH looking for raft-storage-* dirs that do not exist there), then correctly wipes CUSTOM_RAFT_DIR. The redundant traversal is harmless but worth noting. Not a bug.

Nit - Relative path in test constant

private static final String CUSTOM_RAFT_DIR = "./target/custom-raft-storage";

This relies on Maven running tests from the module root, which is standard but implicit. Consider using an absolute path derived from System.getProperty("basedir") or Files.createTempDirectory() for robustness. Low risk in practice.

No concerns about

• Backward compatibility: empty-string default preserves existing layout, zero migration cost.
• Config placement: HA_RAFT_STORAGE_DIRECTORY sits correctly between HA_RAFT_PERSIST_STORAGE and HA_SNAPSHOT_THRESHOLD alphabetically.
• docs/ file: follows the existing project convention (issue-numbered design docs).
• Security: no new attack surface; path is operator-supplied configuration, same as SERVER_ROOT_PATH.
• Performance: the helper is only called during server start/stop, not on the hot path.

Summary

The implementation is clean and correct. The only actionable item is updating RaftBootstrapLateNewerJoinerIT to use the configured directory for its cleanup to stay consistent with the pattern established here. The rest are nits. Ready to merge after that is addressed (or deferred to a follow-up issue).

Code Review

This is a clean, well-scoped feature. The use case (Kubernetes readOnlyRootFilesystem) is clear, backward compatibility is preserved with a zero-migration empty-string default, and the implementation is minimal. A few items worth addressing before merge.

Must Fix

docs/4446-configurable-raft-storage-path.md - internal review log should not be committed

This file reads like an AI-assisted review scaffold rather than user-facing or developer-facing documentation. It includes internal review cycle metadata such as:

**Cycle 1** — head `75cb6b31e`
- gemini: 3 inline comments - ...
- claude: comment about GlobalConfig/ContextConfiguration split ...
...
**Final state:** max-cycles-reached (4/4 cycles used, all actionable items addressed)

This is not suitable for the repository. It exposes internal tooling artifacts. If the intent is to document the feature for operators, the right home is the main docs site or the existing docs/ content pages (not a review-log file named after the issue). Please remove this file before merging.

Should Fix

GlobalConfiguration vs ContextConfiguration split in deleteDatabaseFolders() is a silent maintenance trap

RaftHAServer.getRaftStorageDir() reads from the per-server ContextConfiguration:

// RaftHAServer.java - reads per-server config
String raftDir = configuration.getValueAsString(GlobalConfiguration.HA_RAFT_STORAGE_DIRECTORY);

But BaseRaftHATest.deleteDatabaseFolders() reads from the static global config:

// BaseRaftHATest.java - reads GLOBAL config, not per-server
String raftDir = GlobalConfiguration.HA_RAFT_STORAGE_DIRECTORY.getValueAsString();

This works now only because RaftStorageDirectoryIT overrides deleteDatabaseFolders() to manually clean up. Any future test that sets HA_RAFT_STORAGE_DIRECTORY via config.setValue() in onServerConfiguration() without also overriding deleteDatabaseFolders() will silently leave stale raft dirs behind. The existing Javadoc warns about this, but the warning alone is not enough - this is a trap that is easy to miss.

A safer approach: have BaseRaftHATest track the raft dirs it actually used (recorded in onServerConfiguration() or by querying the running servers), and clean those in deleteDatabaseFolders() instead of re-deriving from the global config.

Minor

isBlank() vs isEmpty() in getRaftStorageDir()

if (raftDir == null || raftDir.isBlank())

This already uses isBlank() - good. But the null guard is dead code: the config entry is declared with String.class, "" default, so getValueAsString() will never return null for this key. The guard is harmless but slightly misleading about what can happen.

Test uses a relative path for CUSTOM_RAFT_DIR

private static final String CUSTOM_RAFT_DIR = "./target/custom-raft-storage";

This works when Maven runs tests from the module root, but is fragile if someone runs the test from the IDE from a different working directory. Using System.getProperty("user.dir") + "/target/custom-raft-storage" or a temporary directory via @TempDir would be more robust.

Consider a startup log/warning for relative paths

The config description recommends an absolute path (/var/lib/arcadedb/raft), but relative paths are silently accepted. A one-line LogManager.instance().log(...) warning when !Paths.get(raftDir).isAbsolute() at startup would help operators catch misconfigurations before they wonder why raft data ended up in an unexpected working directory.

What's Good

• The getRaftStorageDir() helper cleanly eliminates three identical hardcoded path constructions. Good refactoring.
• Backward compatibility is solid - empty default means existing deployments are unaffected.
• The RaftStorageDirectoryIT test is well-structured: positive assertion (dir exists under configured path) and negative assertion (dir does NOT exist under default server root) together give good confidence.
• @Tag("slow") is correctly applied per project convention.
• new File(parent, child) constructor is the right way to build paths here.

Code Review

Overview

This PR cleanly solves a real operational pain point: Kubernetes StatefulSets with readOnlyRootFilesystem: true could not run ArcadeDB HA because Raft storage used a dynamically-named path under the server root. The implementation is focused and well-scoped.

What works well

• Refactoring: 3 duplicated path constructions collapsed into a single getRaftStorageDir() helper - exactly the right approach.
• File(parent, child) constructor: Correct; avoids manual separator handling.
• isBlank(): Right choice over isEmpty() - handles whitespace-only values.
• Backward compatibility: Empty-string default means zero migration cost.
• Test quality: Both a positive assertion (dir exists in custom path) and a negative assertion (dir does NOT exist in server root) - catches both the happy path and the regression scenario.
• @Tag("slow"): Correctly applied per the project annotation convention.

Issues

1. Minor - getRaftStorageDir() should prefer final

Per the project coding standards, final should be used when possible. The current implementation uses a reassignable String raftDir. Suggested refactor using a ternary to allow final:

private File getRaftStorageDir() {
    final String configured = configuration.getValueAsString(GlobalConfiguration.HA_RAFT_STORAGE_DIRECTORY);
    final String parent = (configured == null || configured.isBlank()) ? arcadeServer.getRootPath() : configured;
    return new File(parent, "raft-storage-" + localPeerId);
}

2. Medium - Global vs per-server config mismatch in test cleanup

BaseRaftHATest.deleteDatabaseFolders() reads from GlobalConfiguration.HA_RAFT_STORAGE_DIRECTORY (the global static config), but the test sets the value via onServerConfiguration() which operates on a per-server ContextConfiguration. Those are different objects.

In practice: when RaftStorageDirectoryIT runs, the global config still has the default empty string, so the base-class cleanup resolves to SERVER_ROOT_PATH and tries to delete directories that were never created there - a silent no-op. The actual cleanup happens only because RaftStorageDirectoryIT overrides deleteDatabaseFolders() to explicitly wipe CUSTOM_RAFT_DIR.

The Javadoc warning mitigates this, but the design is fragile: future subclasses that forget to override cleanup will silently leave stale directories on disk. This would be cleaner if the base cleanup tracked the configured paths per server rather than re-reading the global config at teardown, though I understand this may be difficult given the test framework architecture.

3. Suggestion - Warn on relative paths

The config doc says "Set to an absolute path (e.g. /var/lib/arcadedb/raft)", but the code accepts relative paths silently (the test even uses ./target/custom-raft-storage). In production, a relative path resolves relative to the JVM working directory, which may be surprising. A startup log WARNING when a non-blank but non-absolute path is detected would help operators catch misconfiguration early. This is optional - not a blocker.

Summary

The core implementation is correct and well-tested. The final style issue is a minor fix. The global vs per-server config gap in test cleanup is worth being aware of but is adequately documented. Overall this is a clean, focused PR that adds real operational value.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.25%. Comparing base (a882136) to head (eeb00d5).
⚠️ Report is 39 commits behind head on main.

@@            Coverage Diff             @@
##             main    #4447      +/-   ##
==========================================
+ Coverage   65.23%   65.25%   +0.02%     
==========================================
  Files        1612     1612              
  Lines      124319   124320       +1     
  Branches    26908    26908              
==========================================
+ Hits        81100    81126      +26     
+ Misses      31814    31796      -18     
+ Partials    11405    11398       -7     

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