参考BDBLogStorage 基于ChronicleMap、MapDB、H2MVStore、LevelDB-Java实现的日志存储 #1216
Conversation
|
Hi @KnightBlood, welcome to SOFAStack community, Please sign Contributor License Agreement! After you signed CLA, we will automatically sync the status of this pull request in 3 minutes. |
WalkthroughAdds CI workflows for macOS and Windows, upgrades Linux CI job actions, and introduces four new pluggable log-storage modules (LevelDB, H2 MVStore, MapDB, Chronicle Map) with SPI factories, service registrations, implementations, and comprehensive unit tests; updates jraft-extension to include the new modules. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant ServiceLoader
participant SPIFactory as "JRaftServiceFactory (SPI)"
participant LogStorage
App->>ServiceLoader: load(JRaftServiceFactory providers)
ServiceLoader-->>SPIFactory: discover providers (LevelDB/H2/MapDB/Chronicle)
App->>SPIFactory: createLogStorage(uri, raftOptions)
SPIFactory-->>App: return new LogStorage(uri, raftOptions)
App->>LogStorage: init(LogStorageOptions)
LogStorage-->>App: init result
sequenceDiagram
autonumber
actor Raft
participant LogStorage
participant DataDB as "Data Map/DB"
participant ConfDB as "Conf Map/DB"
participant CfgMgr as "ConfigurationManager"
Raft->>LogStorage: appendEntries(entries)
alt entries contain config
LogStorage->>ConfDB: put(index -> config entry)
LogStorage->>CfgMgr: update config manager
end
LogStorage->>DataDB: put(index -> entry bytes)
LogStorage-->>Raft: return count
Raft->>LogStorage: getEntry(index)
LogStorage->>DataDB: get(index)
DataDB-->>LogStorage: bytes
LogStorage-->>Raft: LogEntry
Raft->>LogStorage: truncatePrefix(firstIndexKept)
LogStorage-->>DataDB: schedule background delete < firstIndexKept
LogStorage-->>ConfDB: schedule background delete < firstIndexKept
LogStorage-->>Raft: ok
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 41
♻️ Duplicate comments (2)
jraft-extension/leveldb-log-storage-impl/src/test/java/com/alipay/sofa/jraft/test/TestUtils.java (1)
48-136: Duplicate of other TestUtils — consolidate to a shared test util.jraft-extension/h2mvstore-log-storage-impl/src/test/java/com/alipay/sofa/jraft/test/TestUtils.java (1)
49-136: Duplicate TestUtils across modules — extract to a shared test-jar.
🧹 Nitpick comments (62)
jraft-extension/mapdb-log-storage-impl/pom.xml (2)
16-18: Confirm jraft-core version is managed in the parent.If the parent already manages com.alipay.sofa:jraft-core, consider removing the direct version here or keep it explicit consistently across modules.
25-27: Pin JUnit or migrate to JUnit 5 consistently.Same as the Chronicle Map module: verify version management and testing platform alignment.
jraft-extension/pom.xml (1)
16-19: Module additions look good; consider dependencyManagement centralization.Adding the four storage modules here is consistent. As a follow-up, centralize third-party versions (Chronicle, MapDB, LevelDB, JUnit) under for consistency across modules.
.github/workflows/build-windows.yml (2)
20-24: Use bash consistently on Windows or call the script directly.You set shell: bash but invoke “sh ./tools/check_format.sh”. Prefer “bash ./tools/check_format.sh” for consistency.
- sh ./tools/check_format.sh + bash ./tools/check_format.sh
22-23: Speed up builds with Maven cache and build only needed modules.
- Enable setup-java’s built-in Maven cache.
- Build/reactor only the target module before running its tests to reduce CI time.
- - name: Set up JDK 17 + - name: Set up JDK 17 uses: actions/setup-java@v4 with: distribution: temurin java-version: 17 + cache: maven @@ - mvn clean install -DskipTests=true -Dmaven.javadoc.skip=true -B -V - (mvn --projects jraft-core test \ + mvn -pl jraft-core -am -DskipTests -Dmaven.javadoc.skip=true -B -V install + (mvn --projects jraft-core test \ || mvn --projects jraft-core test \ || mvn --projects jraft-core test)Repeat analogous changes in all jobs.
Also applies to: 38-41, 56-59, 74-77, 92-95, 110-113, 128-131, 146-149, 165-167, 183-185
jraft-extension/leveldb-log-storage-impl/pom.xml (2)
21-29: Avoid redundant LevelDB dependencies; ‘leveldb’ already brings ‘leveldb-api’.Depending on both can cause version skew; prefer only org.iq80.leveldb:leveldb unless you explicitly need api-only at compile time.
<dependency> <groupId>org.iq80.leveldb</groupId> <artifactId>leveldb</artifactId> <version>0.12</version> </dependency> - <dependency> - <groupId>org.iq80.leveldb</groupId> - <artifactId>leveldb-api</artifactId> - <version>0.12</version> - </dependency>
31-33: JUnit version management.Same guidance as other modules: verify version is managed, or pin/migrate consistently.
.github/workflows/build.yml (2)
135-149: Add a JDK 17 “canary” job for Chronicle Map with continue-on-error to surface failures without breaking CI.This helps track the current JDK 17+ breakage the PR mentions.
test_chroniclemap_log_storage_impl: needs: check_format runs-on: ubuntu-latest steps: @@ || mvn --projects jraft-extension/chronicle-map-log-storage-impl test) + + test_chroniclemap_log_storage_impl_jdk17_canary: + needs: check_format + runs-on: ubuntu-latest + continue-on-error: true + steps: + - uses: actions/checkout@v4 + - name: Set up JDK 17 + uses: actions/setup-java@v4 + with: + distribution: temurin + java-version: 17 + cache: maven + - name: Maven Test (canary) + run: mvn clean install -DskipTests=true -Dmaven.javadoc.skip=true -B -V \ + && mvn --projects jraft-extension/chronicle-map-log-storage-impl test
86-86: Remove trailing spaces.Fix YAML lint errors at Line 86 and Line 165.
-86- +86- -165- || mvn --projects jraft-extension/bdb-log-storage-impl test) +165- || mvn --projects jraft-extension/bdb-log-storage-impl test)Also applies to: 165-165
jraft-extension/leveldb-log-storage-impl/src/main/java/com/alipay/sofa/jraft/storage/impl/LevelDBLogStorage.java (3)
254-275: getLastLogIndex is O(N). Consider caching/persisting last index.Maintain a volatile lastLogIndex updated on append/truncate and persist as meta (e.g., meta/lastLogIndex) to avoid scanning on the hot path.
487-516: Broaden exception handling in background truncation.checkState throws IllegalArgumentException on closed DB, which isn’t a DBException. Catch Exception to avoid silent thread death.
- } catch (DBException e) { + } catch (Exception e) { LOG.error("Fail to truncatePrefix {}.", firstIndexKept, e); } finally { this.readLock.unlock(); }
451-453: Remove unused method.isSuccess(Object) is unused; delete to reduce noise.
- protected boolean isSuccess(Object status) { - return status == null || status.equals(Boolean.TRUE); - }.github/workflows/build-macos.yml (2)
135-149: Add a JDK 17 “canary” job for Chronicle Map on macOS.Surface JDK 17 failures without failing CI.
test_chroniclemap_log_storage_impl: needs: check_format runs-on: macos-latest @@ || mvn --projects jraft-extension/chronicle-map-log-storage-impl test) + + test_chroniclemap_log_storage_impl_jdk17_canary: + needs: check_format + runs-on: macos-latest + continue-on-error: true + steps: + - uses: actions/checkout@v4 + - name: Set up JDK 17 + uses: actions/setup-java@v4 + with: + distribution: temurin + java-version: 17 + cache: maven + - name: Maven Test (canary) + run: mvn clean install -DskipTests=true -Dmaven.javadoc.skip=true -B -V \ + && mvn --projects jraft-extension/chronicle-map-log-storage-impl test
86-86: Remove trailing spaces.Fix YAML lint error at Line 86.
-86- +86-jraft-extension/mapdb-log-storage-impl/src/main/resources/META-INF/services/com.alipay.sofa.jraft.JRaftServiceFactory (1)
1-1: Add trailing newline at EOF for POSIX compliance.Avoids subtle tooling diffs.
-com.alipay.sofa.jraft.core.MapDBLogStorageJRaftServiceFactory +com.alipay.sofa.jraft.core.MapDBLogStorageJRaftServiceFactory +jraft-extension/chronicle-map-log-storage-impl/src/main/resources/META-INF/services/com.alipay.sofa.jraft.JRaftServiceFactory (1)
1-1: Add trailing newline at EOF for POSIX compliance.-com.alipay.sofa.jraft.core.ChronicleMapLogStorageJRaftServiceFactory +com.alipay.sofa.jraft.core.ChronicleMapLogStorageJRaftServiceFactory +jraft-extension/leveldb-log-storage-impl/src/main/resources/META-INF/services/com.alipay.sofa.jraft.JRaftServiceFactory (1)
1-1: SPI entries present and correct
All modules (core, BDB, ChronicleMap, H2MVStore, LevelDB, MapDB) include their respectivecom.alipay.sofa.jraft.core.*JRaftServiceFactoryinMETA-INF/services/com.alipay.sofa.jraft.JRaftServiceFactory.
Nit: add a trailing newline tojraft-extension/leveldb-log-storage-impl/src/main/resources/META-INF/services/com.alipay.sofa.jraft.JRaftServiceFactoryfor tooling consistency.jraft-extension/leveldb-log-storage-impl/src/test/java/com/alipay/sofa/jraft/storage/impl/LevelDBLogStorageTest.java (1)
25-27: Optional: add an SPI discovery test for LevelDB factory.A small test that uses ServiceLoader to assert LevelDBLogStorageJRaftServiceFactory is present would catch packaging regressions.
jraft-extension/h2mvstore-log-storage-impl/pom.xml (2)
20-28: Avoid redundant H2 artifacts; likely onlycom.h2database:h2is needed.MVStore is bundled inside the
h2artifact in modern versions; keeping both can introduce duplicate classes and classpath conflicts. Please verify and droph2-mvstoreif unnecessary.Apply this diff if verified:
<dependency> <groupId>com.h2database</groupId> <artifactId>h2</artifactId> <version>2.2.224</version> </dependency> - <dependency> - <groupId>com.h2database</groupId> - <artifactId>h2-mvstore</artifactId> - <version>2.2.224</version> - </dependency>
12-13: Externalize the H2 version into a property for easier upgrades.Keeps versions DRY and consistent across modules.
<name>h2mvstore-log-storage-impl ${project.version}</name> + <properties> + <h2.version>2.2.224</h2.version> + </properties> + <dependencies> <dependency> <groupId>com.alipay.sofa</groupId> <artifactId>jraft-core</artifactId> </dependency> <dependency> <groupId>com.h2database</groupId> <artifactId>h2</artifactId> - <version>2.2.224</version> + <version>${h2.version}</version> </dependency> - <dependency> - <groupId>com.h2database</groupId> - <artifactId>h2-mvstore</artifactId> - <version>2.2.224</version> - </dependency>Also applies to: 14-34
jraft-extension/leveldb-log-storage-impl/src/main/java/com/alipay/sofa/jraft/core/LevelDBLogStorageJRaftServiceFactory.java (2)
19-19: Remove redundant import from the same package.
DefaultJRaftServiceFactoryis in the same package; the import is unnecessary.-import com.alipay.sofa.jraft.core.DefaultJRaftServiceFactory;
30-36: Be mindful of multiple factories with the same SPI priority.If an application brings multiple storage-impl modules on the classpath, competing
@SPI(priority = 1)factories may cause nondeterministic selection depending on loader order. Document this or differentiate priorities.jraft-extension/mapdb-log-storage-impl/src/main/java/com/alipay/sofa/jraft/core/MapDBLogStorageJRaftServiceFactory.java (1)
29-36: LGTM; consider making the factory classfinal.Prevents accidental subclassing of SPI wiring.
-public class MapDBLogStorageJRaftServiceFactory extends DefaultJRaftServiceFactory { +public final class MapDBLogStorageJRaftServiceFactory extends DefaultJRaftServiceFactory {jraft-extension/chronicle-map-log-storage-impl/src/main/java/com/alipay/sofa/jraft/core/ChronicleMapLogStorageJRaftServiceFactory.java (1)
29-36: LGTM; add a guard or log hint for JDK 17+ issues.Given the known JDK 17+ failures in ChronicleMap backend, consider an early log/warn here (or in the storage ctor) to surface actionable guidance.
@Override public LogStorage createLogStorage(String uri, RaftOptions raftOptions) { - return new ChronicleMapLogStorage(uri, raftOptions); + // TODO: remove once JDK17+ compatibility is resolved in ChronicleMapLogStorage + return new ChronicleMapLogStorage(uri, raftOptions); }Please link the failing test names and stack traces to the PR to speed up triage.
jraft-extension/chronicle-map-log-storage-impl/src/test/java/com/alipay/sofa/jraft/storage/BaseStorageTest.java (3)
43-47: Specify UTF-8 and avoid manual separators.Use the charset overload and File(parent, child) to prevent platform-default encoding and path issues.
+import java.nio.charset.StandardCharsets; @@ - File file = new File(this.path + File.separator + "data"); + File file = new File(this.path, "data"); @@ - FileUtils.writeStringToFile(file, data); + FileUtils.writeStringToFile(file, data, StandardCharsets.UTF_8);Also applies to: 19-27
30-36: Prefer @rule TemporaryFolder (JUnit4) or @tempdir (JUnit5) to reduce FS flakiness.Temp helpers handle creation/cleanup reliably across OSes and simplify setup/teardown.
+import org.junit.Rule; +import org.junit.rules.TemporaryFolder; @@ - @Before - public void setup() throws Exception { - this.path = TestUtils.mkTempDir(); - FileUtils.forceMkdir(new File(this.path)); - } + @Rule + public TemporaryFolder tmp = new TemporaryFolder(); + + @Before + public void setup() throws Exception { + this.path = tmp.newFolder().getAbsolutePath(); + } @@ - public void teardown() throws Exception { - FileUtils.deleteDirectory(new File(this.path)); - } + public void teardown() throws Exception { + // TemporaryFolder handles cleanup. + }Also applies to: 38-41
28-31: Consider Path instead of String.Path improves type-safety and eliminates manual separator joins. Not mandatory for this PR.
jraft-extension/h2mvstore-log-storage-impl/src/test/java/com/alipay/sofa/jraft/storage/BaseStorageTest.java (3)
43-47: Specify UTF-8 and avoid manual separators.+import java.nio.charset.StandardCharsets; @@ - File file = new File(this.path + File.separator + "data"); + File file = new File(this.path, "data"); @@ - FileUtils.writeStringToFile(file, data); + FileUtils.writeStringToFile(file, data, StandardCharsets.UTF_8);Also applies to: 19-27
30-36: Use TemporaryFolder/@tempdir for robust temp dirs.+import org.junit.Rule; +import org.junit.rules.TemporaryFolder; @@ + @Rule + public TemporaryFolder tmp = new TemporaryFolder(); @@ - this.path = TestUtils.mkTempDir(); - FileUtils.forceMkdir(new File(this.path)); + this.path = tmp.newFolder().getAbsolutePath(); @@ - FileUtils.deleteDirectory(new File(this.path)); + // handled by TemporaryFolderAlso applies to: 38-41
28-31: De-duplicate BaseStorageTest across modules.Extract to a shared test-jar to avoid four copies drifting.
jraft-extension/mapdb-log-storage-impl/src/test/java/com/alipay/sofa/jraft/storage/BaseStorageTest.java (3)
43-47: Specify UTF-8 and avoid manual separators.+import java.nio.charset.StandardCharsets; @@ - File file = new File(this.path + File.separator + "data"); + File file = new File(this.path, "data"); @@ - FileUtils.writeStringToFile(file, data); + FileUtils.writeStringToFile(file, data, StandardCharsets.UTF_8);Also applies to: 19-27
28-31: Consolidate duplicated test base.Same class exists in multiple modules; move to shared test utility.
30-36: Prefer TemporaryFolder/@tempdir for stability.+import org.junit.Rule; +import org.junit.rules.TemporaryFolder; @@ + @Rule + public TemporaryFolder tmp = new TemporaryFolder(); @@ - this.path = TestUtils.mkTempDir(); - FileUtils.forceMkdir(new File(this.path)); + this.path = tmp.newFolder().getAbsolutePath(); @@ - FileUtils.deleteDirectory(new File(this.path)); + // handled by TemporaryFolderAlso applies to: 38-41
jraft-extension/leveldb-log-storage-impl/src/test/java/com/alipay/sofa/jraft/storage/BaseStorageTest.java (3)
43-47: Specify UTF-8 and avoid manual separators.+import java.nio.charset.StandardCharsets; @@ - File file = new File(this.path + File.separator + "data"); + File file = new File(this.path, "data"); @@ - FileUtils.writeStringToFile(file, data); + FileUtils.writeStringToFile(file, data, StandardCharsets.UTF_8);Also applies to: 19-27
28-31: Consolidate duplicated test base.
30-36: Prefer TemporaryFolder/@tempdir for stability.+import org.junit.Rule; +import org.junit.rules.TemporaryFolder; @@ + @Rule + public TemporaryFolder tmp = new TemporaryFolder(); @@ - this.path = TestUtils.mkTempDir(); - FileUtils.forceMkdir(new File(this.path)); + this.path = tmp.newFolder().getAbsolutePath(); @@ - FileUtils.deleteDirectory(new File(this.path)); + // handled by TemporaryFolderAlso applies to: 38-41
jraft-extension/h2mvstore-log-storage-impl/src/main/java/com/alipay/sofa/jraft/storage/impl/H2MVStoreLogStorage.java (4)
135-143: Avoid materializing key list; iterate keys directly.Iterating keyList() may create a backing list and increase memory usage.
Apply this diff:
- for (byte[] keyBytes : this.confMap.keyList()) { + for (byte[] keyBytes : this.confMap.keySet()) {
454-475: Coordinate background truncation with shutdown.Background deletion can race with shutdown and produce noisy errors. Track the Future and await on shutdown.
Apply this diff:
@@ - private void truncatePrefixInBackground(final long startIndex, final long firstIndexKept) { + private volatile Future<?> pendingTruncate; + private void truncatePrefixInBackground(final long startIndex, final long firstIndexKept) { @@ - ThreadPoolsFactory.runInThread(this.groupId, () -> { + pendingTruncate = ThreadPoolsFactory.runInThread(this.groupId, () -> { @@ }); } @@ public void shutdown() { this.writeLock.lock(); try { + if (pendingTruncate != null) { + try { pendingTruncate.get(); } catch (Exception ignore) { /* no-op */ } + } closeDatabase(); LOG.info("H2MVStoreLogStorage shutdown, the db path is: {}.", this.homePath);Also applies to: 193-202
20-21: Minor: remove unused imports; make 'opened' volatile for clarity.Keeps the class tidy and ensures visibility intent.
Apply this diff:
-import java.io.IOException; @@ -import org.apache.commons.io.IOUtils; @@ - private boolean opened = false; + private volatile boolean opened = false;Also applies to: 29-30, 68-69
396-407: Avoid locking writeLock -> readLock re-entrance in reset().Calling getEntry() while holding writeLock re-enters readLock unnecessarily. Fetch the entry before acquiring writeLock.
Example:
- this.writeLock.lock(); - try { - LogEntry entry = getEntry(nextLogIndex); + LogEntry entry = getEntry(nextLogIndex); + this.writeLock.lock(); + try {jraft-extension/h2mvstore-log-storage-impl/src/test/java/com/alipay/sofa/jraft/storage/impl/BaseLogStorageTest.java (3)
69-74: Set a groupId to isolate background tasks per test suite.Prevents cross-test interference in ThreadPoolsFactory.
Apply this diff:
protected LogStorageOptions newLogStorageOptions() { final LogStorageOptions opts = new LogStorageOptions(); opts.setConfigurationManager(this.confManager); opts.setLogEntryCodecFactory(this.logEntryCodecFactory); + opts.setGroupId("TEST-" + getClass().getSimpleName()); return opts; }
195-205: Gate the 100k-large write test to keep CI stable.Make it opt-in via a system property.
Apply this diff:
+import org.junit.Assume; @@ @Test public void testAppendManyLargeEntries() { + Assume.assumeTrue("Skip unless -DRUN_LONG_STORAGE_BENCH=true", + Boolean.getBoolean("RUN_LONG_STORAGE_BENCH")); final long start = Utils.monotonicMs();Also applies to: 29-33
207-214: Optional: sample verification instead of full scan (perf).Validates correctness with much less I/O.
Example:
- for (int i = 0; i < totalLogs; i++) { + for (int i = 0; i < totalLogs; i += Math.max(1, totalLogs / 1000)) {jraft-extension/mapdb-log-storage-impl/src/test/java/com/alipay/sofa/jraft/storage/impl/BaseLogStorageTest.java (2)
69-74: Set a groupId to isolate background tasks per test suite.Apply this diff:
protected LogStorageOptions newLogStorageOptions() { final LogStorageOptions opts = new LogStorageOptions(); opts.setConfigurationManager(this.confManager); opts.setLogEntryCodecFactory(this.logEntryCodecFactory); + opts.setGroupId("TEST-" + getClass().getSimpleName()); return opts; }
195-206: Gate the 100k-large write test behind a system property to avoid CI timeouts.Apply this diff:
+import org.junit.Assume; @@ @Test public void testAppendManyLargeEntries() { + Assume.assumeTrue("Skip unless -DRUN_LONG_STORAGE_BENCH=true", + Boolean.getBoolean("RUN_LONG_STORAGE_BENCH")); final long start = Utils.monotonicMs();Also applies to: 29-33
jraft-extension/chronicle-map-log-storage-impl/src/test/java/com/alipay/sofa/jraft/storage/impl/BaseLogStorageTest.java (2)
69-74: Set a groupId to isolate background tasks per test suite.Apply this diff:
protected LogStorageOptions newLogStorageOptions() { final LogStorageOptions opts = new LogStorageOptions(); opts.setConfigurationManager(this.confManager); opts.setLogEntryCodecFactory(this.logEntryCodecFactory); + opts.setGroupId("TEST-" + getClass().getSimpleName()); return opts; }
195-205: Gate the 100k-large write test behind a system property to avoid CI flakiness.Apply this diff:
+import org.junit.Assume; @@ @Test public void testAppendManyLargeEntries() { + Assume.assumeTrue("Skip unless -DRUN_LONG_STORAGE_BENCH=true", + Boolean.getBoolean("RUN_LONG_STORAGE_BENCH")); final long start = Utils.monotonicMs();Also applies to: 29-33
jraft-extension/chronicle-map-log-storage-impl/src/test/java/com/alipay/sofa/jraft/test/TestUtils.java (1)
33-41: Remove unused imports and pin charset to UTF-8.Prevents warnings and platform-dependent behavior.
Apply this diff:
import com.alipay.sofa.jraft.JRaftUtils; import com.alipay.sofa.jraft.conf.ConfigurationEntry; import com.alipay.sofa.jraft.entity.EnumOutter; import com.alipay.sofa.jraft.entity.LogEntry; import com.alipay.sofa.jraft.entity.LogId; import com.alipay.sofa.jraft.entity.PeerId; -import com.alipay.sofa.jraft.rpc.RpcRequests; -import com.alipay.sofa.jraft.util.Endpoint; +import java.nio.charset.StandardCharsets; @@ - if (i > 0) { - entry.setData(ByteBuffer.wrap(String.valueOf(i).getBytes())); + if (i > 0) { + entry.setData(ByteBuffer.wrap(String.valueOf(i).getBytes(StandardCharsets.UTF_8))); }Also applies to: 97-101
jraft-extension/mapdb-log-storage-impl/src/test/java/com/alipay/sofa/jraft/test/TestUtils.java (3)
97-99: Use explicit UTF-8 to avoid platform-dependent encodings.- entry.setData(ByteBuffer.wrap(String.valueOf(i).getBytes())); + entry.setData(ByteBuffer.wrap(String.valueOf(i).getBytes(java.nio.charset.StandardCharsets.UTF_8)));
108-123: Stop scanning once an IPv4 address is found.Avoid unnecessary traversal and accidental override.
- while (interfaces.hasMoreElements()) { + while (interfaces.hasMoreElements()) { NetworkInterface iface = interfaces.nextElement(); // filters out 127.0.0.1 and inactive interfaces if (iface.isLoopback() || !iface.isUp()) { continue; } Enumeration<InetAddress> addresses = iface.getInetAddresses(); while (addresses.hasMoreElements()) { InetAddress addr = addresses.nextElement(); if (addr instanceof Inet4Address) { - ip = addr.getHostAddress(); - break; + return addr.getHostAddress(); } } }
49-136: Deduplicate TestUtils across extension modules.Identical helpers exist in chronicle-map, h2mvstore, and leveldb modules. Extract a shared test-jar (e.g., jraft-extension-test-utils) and depend on it in tests.
jraft-extension/leveldb-log-storage-impl/src/test/java/com/alipay/sofa/jraft/test/TestUtils.java (2)
97-99: Pin encoding to UTF-8.- entry.setData(ByteBuffer.wrap(String.valueOf(i).getBytes())); + entry.setData(ByteBuffer.wrap(String.valueOf(i).getBytes(java.nio.charset.StandardCharsets.UTF_8)));
108-123: Return on first IPv4 hit.- if (addr instanceof Inet4Address) { - ip = addr.getHostAddress(); - break; - } + if (addr instanceof Inet4Address) { + return addr.getHostAddress(); + }jraft-extension/h2mvstore-log-storage-impl/src/test/java/com/alipay/sofa/jraft/test/TestUtils.java (2)
97-99: Use UTF-8 explicitly.- entry.setData(ByteBuffer.wrap(String.valueOf(i).getBytes())); + entry.setData(ByteBuffer.wrap(String.valueOf(i).getBytes(java.nio.charset.StandardCharsets.UTF_8)));
108-123: Short-circuit when an IPv4 is found.- if (addr instanceof Inet4Address) { - ip = addr.getHostAddress(); - break; - } + if (addr instanceof Inet4Address) { + return addr.getHostAddress(); + }jraft-extension/leveldb-log-storage-impl/src/test/java/com/alipay/sofa/jraft/storage/impl/BaseLogStorageTest.java (1)
196-215: Gate the heavy 100k x 16KB performance test behind a system property.Prevents CI flakiness/timeouts while keeping the test available locally.
@Test public void testAppendManyLargeEntries() { + if (!Boolean.getBoolean("RUN_SLOW_TESTS")) { + return; + } final long start = Utils.monotonicMs(); final int totalLogs = 100000; final int logSize = 16 * 1024; final int batch = 100;jraft-extension/chronicle-map-log-storage-impl/src/main/java/com/alipay/sofa/jraft/storage/impl/ChronicleMapLogStorage.java (4)
423-426: Ensure firstLogIndex is loaded before comparison.- checkState(); + checkState(); + if (!this.hasLoadFirstLogIndex) { + loadFirstLogIndex(); + } if (this.hasLoadFirstLogIndex && index < this.firstLogIndex) { return null; }
76-95: Unused ‘sync’ option.Either honor RaftOptions.isSync() (e.g., fsync metadata) or remove the field.
Would you like me to wire sync to durable writes for meta updates (e.g., after saveFirstLogIndex/truncations)?
268-286: Minor: remove unused local variable.
long lastLogIndex = 0;is unused in loadLastLogIndex().- long lastLogIndex = 0; for (byte[] keyBytes : this.defaultMap.keySet()) {
642-657: Background deletion uses groupId; ensure it’s non-null.If unset, tasks may land in a shared pool. Consider defaulting groupId in init when opts.getGroupId() is null.
- ThreadPoolsFactory.runInThread(this.groupId, () -> { + ThreadPoolsFactory.runInThread(this.groupId != null ? this.groupId : "default", () -> {jraft-extension/mapdb-log-storage-impl/src/main/java/com/alipay/sofa/jraft/storage/impl/MapDBLogStorage.java (2)
112-118: Use standard JVM properties for architecture detection.The current implementation relies on non-standard system properties to detect JVM architecture. Consider using standard properties for better portability.
private boolean is64BitJVM() { - String model = System.getProperty("sun.arch.data.model"); - if (model != null && model.equals("64")) { - return true; - } - return false; + String arch = System.getProperty("os.arch"); + return arch != null && (arch.contains("64") || arch.equals("amd64") || arch.equals("x86_64")); }
186-195: Ensure thread pool is properly registered for lifecycle management.The flush executor service should be registered with ThreadPoolsFactory for proper lifecycle management.
if (!this.sync) { this.flushExecutorService = ThreadPoolUtil.newScheduledBuilder() .poolName("mapdb-flush-executor") .enableMetric(true) .coreThreads(1) .threadFactory(new NamedThreadFactory("MapDB-Flush-Thread-", true)) .build(); + // Register for proper lifecycle management + ThreadPoolsFactory.registerThreadPool(this.groupId, this.flushExecutorService); this.flushScheduledFuture = this.flushExecutorService.scheduleWithFixedDelay(this::flushDatabase, flushIntervalMs, flushIntervalMs, TimeUnit.MILLISECONDS); }
.github/workflows/build-macos.yml
Outdated
| - uses: actions/checkout@v2 | ||
| - name: Set up JDK 17 | ||
| uses: actions/setup-java@v3 | ||
| with: | ||
| distribution: temurin | ||
| java-version: 17 | ||
| - name: Maven Test | ||
| run: mvn clean install -DskipTests=true -Dmaven.javadoc.skip=true -B -V | ||
| && (mvn --projects jraft-extension/leveldb-log-storage-impl test | ||
| || mvn --projects jraft-extension/leveldb-log-storage-impl test | ||
| || mvn --projects jraft-extension/leveldb-log-storage-impl test) |
There was a problem hiding this comment.
Upgrade actions to v4 and cache Maven.
Same rationale as Linux workflow; do this across all new jobs in this file.
- - uses: actions/checkout@v2
- - name: Set up JDK 17
- uses: actions/setup-java@v3
+ - uses: actions/checkout@v4
+ - name: Set up JDK 17
+ uses: actions/setup-java@v4
with:
distribution: temurin
java-version: 17
+ cache: maven📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/checkout@v2 | |
| - name: Set up JDK 17 | |
| uses: actions/setup-java@v3 | |
| with: | |
| distribution: temurin | |
| java-version: 17 | |
| - name: Maven Test | |
| run: mvn clean install -DskipTests=true -Dmaven.javadoc.skip=true -B -V | |
| && (mvn --projects jraft-extension/leveldb-log-storage-impl test | |
| || mvn --projects jraft-extension/leveldb-log-storage-impl test | |
| || mvn --projects jraft-extension/leveldb-log-storage-impl test) | |
| - uses: actions/checkout@v4 | |
| - name: Set up JDK 17 | |
| uses: actions/setup-java@v4 | |
| with: | |
| distribution: temurin | |
| java-version: 17 | |
| cache: maven | |
| - name: Maven Test | |
| run: mvn clean install -DskipTests=true -Dmaven.javadoc.skip=true -B -V | |
| && (mvn --projects jraft-extension/leveldb-log-storage-impl test | |
| || mvn --projects jraft-extension/leveldb-log-storage-impl test | |
| || mvn --projects jraft-extension/leveldb-log-storage-impl test) |
🧰 Tools
🪛 actionlint (1.7.7)
91-91: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
93-93: the runner of "actions/setup-java@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In .github/workflows/build-macos.yml around lines 91–101, update the workflow to
use actions v4 and add Maven caching: change uses: actions/checkout@v2 to
actions/checkout@v4 and uses: actions/setup-java@v3 to actions/setup-java@v4,
then add a step before the Maven Test step that uses actions/cache@v4 to cache
the Maven local repository (path: ~/.m2/repository) with a key like ${{
runner.os }}-m2-${{ hashFiles('**/pom.xml') }} and a restore-keys fallback of
${{ runner.os }}-m2-, so the cache is restored before running mvn clean install
and speeds subsequent builds.
.github/workflows/build-macos.yml
Outdated
| - uses: actions/checkout@v2 | ||
| - name: Set up JDK 8 | ||
| uses: actions/setup-java@v3 | ||
| with: | ||
| distribution: temurin | ||
| java-version: 8 | ||
| - name: Maven Test | ||
| run: mvn clean install -DskipTests=true -Dmaven.javadoc.skip=true -B -V | ||
| && (mvn --projects jraft-extension/chronicle-map-log-storage-impl test | ||
| || mvn --projects jraft-extension/chronicle-map-log-storage-impl test | ||
| || mvn --projects jraft-extension/chronicle-map-log-storage-impl test) |
There was a problem hiding this comment.
Chronicle Map macOS job: keep JDK 8 but move actions to v4 and add cache.
- - uses: actions/checkout@v2
+ - uses: actions/checkout@v4
- name: Set up JDK 8
- uses: actions/setup-java@v3
+ uses: actions/setup-java@v4
with:
distribution: temurin
java-version: 8
+ cache: maven🧰 Tools
🪛 actionlint (1.7.7)
139-139: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
141-141: the runner of "actions/setup-java@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| test_chroniclemap_log_storage_impl: | ||
| needs: check_format | ||
| runs-on: windows-latest | ||
| steps: | ||
| - uses: actions/checkout@v2 | ||
| - name: Set up JDK 8 | ||
| uses: actions/setup-java@v3 | ||
| with: | ||
| distribution: temurin | ||
| java-version: 8 | ||
| - name: Maven Test | ||
| shell: bash | ||
| run: | | ||
| mvn clean install -DskipTests=true -Dmaven.javadoc.skip=true -B -V | ||
| (mvn --projects jraft-extension/chronicle-map-log-storage-impl test \ | ||
| || mvn --projects jraft-extension/chronicle-map-log-storage-impl test \ | ||
| || mvn --projects jraft-extension/chronicle-map-log-storage-impl test) | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a JDK 17 run for ChronicleMap with add-opens to surface failures now.
Keep the JDK 8 job, but add another job for JDK 17 using required add-opens so failures/logs are visible in CI, aiding the transition.
Example job (outside selected range):
test_chroniclemap_log_storage_impl_jdk17:
needs: check_format
runs-on: windows-latest
steps:
- uses: actions/checkout@v4
- name: Set up JDK 17
uses: actions/setup-java@v4
with:
distribution: temurin
java-version: 17
cache: maven
- name: Maven Test (JDK17 with add-opens)
shell: bash
env:
MAVEN_OPTS: >-
--add-exports=java.base/jdk.internal.ref=ALL-UNNAMED
--add-exports=java.base/sun.nio.ch=ALL-UNNAMED
--add-opens=java.base/java.lang=ALL-UNNAMED
--add-opens=java.base/java.lang.reflect=ALL-UNNAMED
--add-opens=java.base/java.nio=ALL-UNNAMED
--add-opens=java.base/sun.nio.ch=ALL-UNNAMED
run: |
mvn -pl jraft-extension/chronicle-map-log-storage-impl -am -DskipTests -B -V install
mvn --projects jraft-extension/chronicle-map-log-storage-impl test🧰 Tools
🪛 actionlint (1.7.7)
155-155: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
157-157: the runner of "actions/setup-java@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
.github/workflows/build-windows.yml lines 151-168: the current workflow only
runs the ChronicleMap test job on JDK 8; add a separate JDK 17 job (keep the
existing JDK8 job) that runs on windows-latest, checks out the repo, uses
actions/setup-java@v4 to install Temurin 17 (with maven cache), sets MAVEN_OPTS
with the required --add-exports and --add-opens entries shown in the comment,
and runs the Maven install/test for
jraft-extension/chronicle-map-log-storage-impl so failures and logs under JDK17
are visible in CI; place this new job alongside the existing job (outside the
selected range) and ensure the job name and needs are consistent with the
workflow.
.github/workflows/build.yml
Outdated
| - uses: actions/checkout@v2 | ||
| - name: Set up JDK 17 | ||
| uses: actions/setup-java@v3 | ||
| with: | ||
| distribution: temurin | ||
| java-version: 17 | ||
| - name: Maven Test | ||
| run: mvn clean install -DskipTests=true -Dmaven.javadoc.skip=true -B -V | ||
| && (mvn --projects jraft-extension/leveldb-log-storage-impl test | ||
| || mvn --projects jraft-extension/leveldb-log-storage-impl test | ||
| || mvn --projects jraft-extension/leveldb-log-storage-impl test) |
There was a problem hiding this comment.
Use current actions and enable Maven cache (fixes actionlint, speeds up runs).
actions/checkout@v2 and actions/setup-java@v3 are deprecated on the current runners. Upgrade to v4 and enable cache. Apply the same change to all new jobs in this hunk (lines 91-101, 107-117, 123-133, 139-149, 155-165).
- - uses: actions/checkout@v2
- - name: Set up JDK 17
- uses: actions/setup-java@v3
+ - uses: actions/checkout@v4
+ - name: Set up JDK 17
+ uses: actions/setup-java@v4
with:
distribution: temurin
java-version: 17
+ cache: maven📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/checkout@v2 | |
| - name: Set up JDK 17 | |
| uses: actions/setup-java@v3 | |
| with: | |
| distribution: temurin | |
| java-version: 17 | |
| - name: Maven Test | |
| run: mvn clean install -DskipTests=true -Dmaven.javadoc.skip=true -B -V | |
| && (mvn --projects jraft-extension/leveldb-log-storage-impl test | |
| || mvn --projects jraft-extension/leveldb-log-storage-impl test | |
| || mvn --projects jraft-extension/leveldb-log-storage-impl test) | |
| - uses: actions/checkout@v4 | |
| - name: Set up JDK 17 | |
| uses: actions/setup-java@v4 | |
| with: | |
| distribution: temurin | |
| java-version: 17 | |
| cache: maven | |
| - name: Maven Test | |
| run: mvn clean install -DskipTests=true -Dmaven.javadoc.skip=true -B -V | |
| && (mvn --projects jraft-extension/leveldb-log-storage-impl test | |
| || mvn --projects jraft-extension/leveldb-log-storage-impl test | |
| || mvn --projects jraft-extension/leveldb-log-storage-impl test) |
🧰 Tools
🪛 actionlint (1.7.7)
91-91: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
93-93: the runner of "actions/setup-java@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In .github/workflows/build.yml around lines 91 to 101 (and repeat for the other
hunk ranges 107-117, 123-133, 139-149, 155-165), update deprecated actions and
enable Maven caching: replace actions/checkout@v2 with actions/checkout@v4 and
actions/setup-java@v3 with actions/setup-java@v4, and enable the built‑in Maven
cache feature (e.g., set cache: 'maven' or configure the maven cache inputs per
setup-java v4) or add an actions/cache step for ~/.m2/repository to speed builds
and satisfy actionlint; apply the same replacements and cache addition to all
corresponding job sections in this file.
| private boolean bufferEntry(LogEntry entry) { | ||
| synchronized (bufferLock) { | ||
| writeBuffer.add(entry); | ||
|
|
||
| // 如果达到批处理大小，立即提交 | ||
| if (writeBuffer.size() >= batchSize) { | ||
| commitBuffer(); | ||
| this.db.commit(); | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| // 标记需要定期刷新 | ||
| needFlush = true; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Potential data loss in async mode if process crashes.
In bufferEntry, entries are added to the buffer but not immediately persisted. If the process crashes before the next flush, these entries will be lost.
Consider:
- Implementing write-ahead logging
- Using MapDB's transaction support even in async mode
- Documenting the durability trade-offs clearly
private boolean bufferEntry(LogEntry entry) {
synchronized (bufferLock) {
writeBuffer.add(entry);
// 如果达到批处理大小，立即提交
if (writeBuffer.size() >= batchSize) {
commitBuffer();
this.db.commit();
return true;
}
+
+ // Consider forcing flush for critical entries (e.g., configuration changes)
+ if (entry.getType() == EntryType.ENTRY_TYPE_CONFIGURATION) {
+ commitBuffer();
+ this.db.commit();
+ return true;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private boolean bufferEntry(LogEntry entry) { | |
| synchronized (bufferLock) { | |
| writeBuffer.add(entry); | |
| // 如果达到批处理大小，立即提交 | |
| if (writeBuffer.size() >= batchSize) { | |
| commitBuffer(); | |
| this.db.commit(); | |
| return true; | |
| } | |
| } | |
| // 标记需要定期刷新 | |
| needFlush = true; | |
| return true; | |
| } | |
| private boolean bufferEntry(LogEntry entry) { | |
| synchronized (bufferLock) { | |
| writeBuffer.add(entry); | |
| // 如果达到批处理大小，立即提交 | |
| if (writeBuffer.size() >= batchSize) { | |
| commitBuffer(); | |
| this.db.commit(); | |
| return true; | |
| } | |
| // Consider forcing flush for critical entries (e.g., configuration changes) | |
| if (entry.getType() == EntryType.ENTRY_TYPE_CONFIGURATION) { | |
| commitBuffer(); | |
| this.db.commit(); | |
| return true; | |
| } | |
| } | |
| // 标记需要定期刷新 | |
| needFlush = true; | |
| return true; | |
| } |
| synchronized (writeBuffer) { | ||
| int totalAdded = 0; | ||
| int bufferSize = writeBuffer.size(); | ||
| for (LogEntry entry : entries) { | ||
| writeBuffer.add(entry); | ||
| totalAdded++; | ||
| bufferSize++; | ||
|
|
||
| // 如果达到批处理大小，立即提交 | ||
| if (bufferSize >= batchSize) { | ||
| commitBuffer(); | ||
| bufferSize = 0; | ||
| } | ||
| } | ||
|
|
||
| // 最后提交一次剩余的日志 | ||
| if (bufferSize > 0) { | ||
| commitBuffer(); | ||
| this.db.commit(); | ||
| } else { | ||
| this.db.commit(); | ||
| } | ||
|
|
||
| return totalAdded; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix synchronization issue in bufferEntries method.
The method synchronizes on writeBuffer but should synchronize on bufferLock for consistency with other buffer operations.
private int bufferEntries(List<LogEntry> entries) {
- synchronized (writeBuffer) {
+ synchronized (bufferLock) {
int totalAdded = 0;
int bufferSize = writeBuffer.size();
for (LogEntry entry : entries) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| synchronized (writeBuffer) { | |
| int totalAdded = 0; | |
| int bufferSize = writeBuffer.size(); | |
| for (LogEntry entry : entries) { | |
| writeBuffer.add(entry); | |
| totalAdded++; | |
| bufferSize++; | |
| // 如果达到批处理大小，立即提交 | |
| if (bufferSize >= batchSize) { | |
| commitBuffer(); | |
| bufferSize = 0; | |
| } | |
| } | |
| // 最后提交一次剩余的日志 | |
| if (bufferSize > 0) { | |
| commitBuffer(); | |
| this.db.commit(); | |
| } else { | |
| this.db.commit(); | |
| } | |
| return totalAdded; | |
| } | |
| } | |
| private int bufferEntries(List<LogEntry> entries) { | |
| synchronized (bufferLock) { | |
| int totalAdded = 0; | |
| int bufferSize = writeBuffer.size(); | |
| for (LogEntry entry : entries) { | |
| writeBuffer.add(entry); | |
| totalAdded++; | |
| bufferSize++; | |
| // 如果达到批处理大小，立即提交 | |
| if (bufferSize >= batchSize) { | |
| commitBuffer(); | |
| bufferSize = 0; | |
| } | |
| } | |
| // 最后提交一次剩余的日志 | |
| if (bufferSize > 0) { | |
| commitBuffer(); | |
| this.db.commit(); | |
| } else { | |
| this.db.commit(); | |
| } | |
| return totalAdded; | |
| } | |
| } |
🤖 Prompt for AI Agents
In
jraft-extension/mapdb-log-storage-impl/src/main/java/com/alipay/sofa/jraft/storage/impl/MapDBLogStorage.java
around lines 593 to 618, the method currently synchronizes on writeBuffer but
other buffer operations use bufferLock; change the synchronized block to use
bufferLock instead of writeBuffer to ensure consistent locking, i.e., replace
synchronized(writeBuffer) { ... } with synchronized(bufferLock) { ... }, keeping
the same internal logic (adding entries, batching, commitBuffer calls and
db.commit()) so all buffer manipulations use the same lock.
| final long lastLogIndex = getLastLogIndex(); | ||
| for (long index = lastIndexKept + 1; index <= lastLogIndex; index++) { | ||
| byte[] key = getKeyBytes(index); | ||
| // Delete it first; otherwise, it may never be deleted | ||
| this.confMap.remove(key); // Delete it first; otherwise, it may never be deleted | ||
| this.defaultMap.remove(key); | ||
| } | ||
| this.db.commit(); | ||
| return true; | ||
| } catch (Exception e) { | ||
| LOG.error("Fail to truncateSuffix {}.", lastIndexKept, e); | ||
| } finally { | ||
| this.readLock.unlock(); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inefficient truncateSuffix implementation.
The current implementation deletes entries one by one in a loop, which is inefficient for large ranges.
Consider batching the deletions or using MapDB's bulk removal operations:
public boolean truncateSuffix(long lastIndexKept) {
this.readLock.lock();
try {
checkState();
final long lastLogIndex = getLastLogIndex();
- for (long index = lastIndexKept + 1; index <= lastLogIndex; index++) {
- byte[] key = getKeyBytes(index);
- // Delete it first; otherwise, it may never be deleted
- this.confMap.remove(key); // Delete it first; otherwise, it may never be deleted
- this.defaultMap.remove(key);
- }
+ // Batch delete operations for better performance
+ List<byte[]> keysToRemove = new ArrayList<>();
+ for (long index = lastIndexKept + 1; index <= lastLogIndex; index++) {
+ keysToRemove.add(getKeyBytes(index));
+ }
+
+ // Remove all keys in batch
+ for (byte[] key : keysToRemove) {
+ this.confMap.remove(key);
+ this.defaultMap.remove(key);
+ }
+
this.db.commit();
return true;Committable suggestion skipped: line range outside the PR's diff.
| LogEntry entry = getEntry(nextLogIndex); | ||
| closeDatabase(); | ||
| FileUtils.deleteDirectory(new File(this.homePath)); | ||
| initAndLoad(null); | ||
| if (entry == null) { | ||
| entry = new LogEntry(); | ||
| entry.setType(EntryType.ENTRY_TYPE_NO_OP); | ||
| entry.setId(new LogId(nextLogIndex, 0)); | ||
| LOG.warn("Entry not found for nextLogIndex {} when reset.", nextLogIndex); | ||
| } | ||
| return appendEntry(entry); | ||
| } catch (Exception e) { | ||
| LOG.error("Fail to reset next log index.", e); | ||
| } finally { | ||
| this.writeLock.unlock(); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential atomicity issue in reset method.
The reset method retrieves an entry, closes the database, deletes files, and then re-appends the entry. This sequence is not atomic and could lead to data inconsistency if interrupted.
Consider implementing this as an atomic operation or adding recovery logic:
public boolean reset(long nextLogIndex) {
if (nextLogIndex <= 0) {
throw new IllegalArgumentException("Invalid next log index.");
}
this.writeLock.lock();
try {
+ // Save the entry before any destructive operations
LogEntry entry = getEntry(nextLogIndex);
+ if (entry == null) {
+ entry = new LogEntry();
+ entry.setType(EntryType.ENTRY_TYPE_NO_OP);
+ entry.setId(new LogId(nextLogIndex, 0));
+ LOG.warn("Entry not found for nextLogIndex {} when reset.", nextLogIndex);
+ }
+
+ // Make a defensive copy to ensure data integrity
+ final LogEntry entryCopy = entry.copy();
+
closeDatabase();
FileUtils.deleteDirectory(new File(this.homePath));
initAndLoad(null);
- if (entry == null) {
- entry = new LogEntry();
- entry.setType(EntryType.ENTRY_TYPE_NO_OP);
- entry.setId(new LogId(nextLogIndex, 0));
- LOG.warn("Entry not found for nextLogIndex {} when reset.", nextLogIndex);
- }
- return appendEntry(entry);
+ return appendEntry(entryCopy);🏁 Script executed:
#!/bin/bash
# Check if LogEntry has a copy method
ast-grep --pattern 'class LogEntry {
$$$
copy() {
$$$
}
$$$
}'Length of output: 76
🏁 Script executed:
#!/bin/bash
# Locate LogEntry class definition
rg -nP 'class\s+LogEntry\b' --type java -C2
# Check for copy() method declarations in LogEntry or related classes
rg -nP 'copy\s*\(' --type java -C2Length of output: 61445
Implement a deep‐clone for LogEntry before destructive reset
- Move the
getEntry(nextLogIndex)call (and null‐handling) beforecloseDatabase()/file deletion to capture the entry’s state. - Because
LogEntrydoes not implement acopy()method, add acopy()or similar deep‐clone to preserve all fields. - Wrap the deleteDirectory + initAndLoad sequence in transactional or recovery logic so an interrupted reset cannot leave the log in an inconsistent state.
| public static String mkTempDir() { | ||
| return Paths.get(System.getProperty("java.io.tmpdir", "/tmp"), "jraft_test_" + System.nanoTime()).toString(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Create the temp directory, don’t just return the path.
Tests that expect a writable directory can fail if it doesn’t exist.
- public static String mkTempDir() {
- return Paths.get(System.getProperty("java.io.tmpdir", "/tmp"), "jraft_test_" + System.nanoTime()).toString();
- }
+ public static String mkTempDir() {
+ final java.nio.file.Path p = Paths.get(System.getProperty("java.io.tmpdir", "/tmp"),
+ "jraft_test_" + System.nanoTime());
+ try { java.nio.file.Files.createDirectories(p); } catch (Exception ignore) {}
+ return p.toString();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static String mkTempDir() { | |
| return Paths.get(System.getProperty("java.io.tmpdir", "/tmp"), "jraft_test_" + System.nanoTime()).toString(); | |
| } | |
| public static String mkTempDir() { | |
| final java.nio.file.Path p = Paths.get(System.getProperty("java.io.tmpdir", "/tmp"), | |
| "jraft_test_" + System.nanoTime()); | |
| try { java.nio.file.Files.createDirectories(p); } catch (Exception ignore) {} | |
| return p.toString(); | |
| } |
🤖 Prompt for AI Agents
In
jraft-extension/mapdb-log-storage-impl/src/test/java/com/alipay/sofa/jraft/test/TestUtils.java
around lines 70 to 72, the mkTempDir method currently only returns a path string
but does not ensure the directory exists; update it to create the directory
before returning: build the Path from System.getProperty("java.io.tmpdir") and
the generated folder name, call Files.createDirectories(path) (or equivalent) to
ensure the directory and any parents exist, and then return path.toString();
propagate or wrap IOExceptions (e.g., throw a RuntimeException) so test callers
fail fast if directory creation fails.
| try { | ||
| Iterator<byte[]> iterator = this.defaultMap.getKeys().iterator(); | ||
| byte[] firstKey = null; | ||
| long minIndex = Long.MAX_VALUE; | ||
| boolean hasKey = false; | ||
|
|
||
| // 遍历所有键，找到最小的索引值 | ||
| while (iterator.hasNext()) { | ||
| hasKey = true; | ||
| byte[] key = iterator.next(); | ||
| if (key != null && key.length == Long.BYTES) { | ||
| long index = Bits.getLong(key, 0); | ||
| if (index < minIndex) { | ||
| minIndex = index; | ||
| firstKey = key; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (hasKey && firstKey != null) { | ||
| final long firstLogIndex = minIndex; | ||
| saveFirstLogIndex(firstLogIndex); | ||
| setFirstLogIndex(firstLogIndex); | ||
| return firstLogIndex; | ||
| } | ||
| } catch (Exception e) { | ||
| LOG.error("Fail to get first log index.", e); | ||
| } | ||
| } finally { | ||
| this.readLock.unlock(); | ||
| } | ||
| return 1L; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize getFirstLogIndex with index caching.
The current implementation iterates through all keys to find the minimum index on every call when the index hasn't been loaded. This is inefficient for large datasets.
Consider:
- Maintaining the first index in memory and updating it during truncate operations
- Using MapDB's NavigableSet for efficient min/max operations
- Only scanning keys once during initialization
try {
- Iterator<byte[]> iterator = this.defaultMap.getKeys().iterator();
- byte[] firstKey = null;
- long minIndex = Long.MAX_VALUE;
- boolean hasKey = false;
-
- // 遍历所有键，找到最小的索引值
- while (iterator.hasNext()) {
- hasKey = true;
- byte[] key = iterator.next();
- if (key != null && key.length == Long.BYTES) {
- long index = Bits.getLong(key, 0);
- if (index < minIndex) {
- minIndex = index;
- firstKey = key;
- }
- }
- }
-
- if (hasKey && firstKey != null) {
- final long firstLogIndex = minIndex;
+ // Use NavigableSet for efficient min operation if available
+ // or consider caching this value
+ byte[] firstKey = this.defaultMap.firstKey();
+ if (firstKey != null && firstKey.length == Long.BYTES) {
+ final long firstLogIndex = Bits.getLong(firstKey, 0);
saveFirstLogIndex(firstLogIndex);
setFirstLogIndex(firstLogIndex);
return firstLogIndex;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| Iterator<byte[]> iterator = this.defaultMap.getKeys().iterator(); | |
| byte[] firstKey = null; | |
| long minIndex = Long.MAX_VALUE; | |
| boolean hasKey = false; | |
| // 遍历所有键，找到最小的索引值 | |
| while (iterator.hasNext()) { | |
| hasKey = true; | |
| byte[] key = iterator.next(); | |
| if (key != null && key.length == Long.BYTES) { | |
| long index = Bits.getLong(key, 0); | |
| if (index < minIndex) { | |
| minIndex = index; | |
| firstKey = key; | |
| } | |
| } | |
| } | |
| if (hasKey && firstKey != null) { | |
| final long firstLogIndex = minIndex; | |
| saveFirstLogIndex(firstLogIndex); | |
| setFirstLogIndex(firstLogIndex); | |
| return firstLogIndex; | |
| } | |
| } catch (Exception e) { | |
| LOG.error("Fail to get first log index.", e); | |
| } | |
| } finally { | |
| this.readLock.unlock(); | |
| } | |
| return 1L; | |
| } | |
| try { | |
| // Use NavigableMap for efficient min operation if available | |
| // or consider caching this value | |
| byte[] firstKey = this.defaultMap.firstKey(); | |
| if (firstKey != null && firstKey.length == Long.BYTES) { | |
| final long firstLogIndex = Bits.getLong(firstKey, 0); | |
| saveFirstLogIndex(firstLogIndex); | |
| setFirstLogIndex(firstLogIndex); | |
| return firstLogIndex; | |
| } | |
| } catch (Exception e) { | |
| LOG.error("Fail to get first log index.", e); | |
| } finally { | |
| this.readLock.unlock(); | |
| } | |
| return 1L; |
🤖 Prompt for AI Agents
In
jraft-extension/mapdb-log-storage-impl/src/main/java/com/alipay/sofa/jraft/storage/impl/MapDBLogStorage.java
around lines 386 to 418, the method currently scans all keys to find the minimum
index each time the cached first index is not loaded; replace that by
introducing an in-memory cached firstLogIndex that is loaded once at
initialization (or first access) and persisted via saveFirstLogIndex, update
this cache on any operations that can change the first index
(truncatePrefix/truncateSuffix/deleteRange), and avoid repeated full scans;
optionally switch the underlying key set to a MapDB NavigableSet (or use the
DB's firstKey API) to get min/max in O(1) when scanning is unavoidable; ensure
proper locking when reading/updating the cached value and fall back to a single
scan only during initialization if the persisted value is missing.
update actions/setup-java v3 to v5
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/build.yml (2)
34-37: Build only what’s needed (-pl -am) instead of “install everything then test one module”.Current flow compiles the entire repo each job, then runs a single-module test. Use Maven’s -pl/-am to build deps and the target module only; this materially reduces CI time.
- - name: Maven Test - run: mvn clean install -DskipTests=true -Dmaven.javadoc.skip=true -B -V - && (mvn --projects jraft-extension/leveldb-log-storage-impl test - || mvn --projects jraft-extension/leveldb-log-storage-impl test - || mvn --projects jraft-extension/leveldb-log-storage-impl test) + - name: Maven Test (targeted) + run: mvn -pl jraft-extension/leveldb-log-storage-impl -am \ + -Dmaven.javadoc.skip=true -B -V testReplicate for the other module-specific jobs.
Also applies to: 50-53, 66-69, 82-85, 98-101, 114-117, 130-133, 146-149, 162-165
35-37: Replace all ad-hoccmd || cmd || cmdretry patterns with a structured retry step
The repetition in lines 35–37, 51–53, 67–69, 83–85, 99–101, 115–117, 131–133, 147–149, and 163–165 hides logs and lacks backoff/timeout controls; refactor these to use a retry action (e.g., nick-fields/retry@v3) or a bash loop with backoff.
🧹 Nitpick comments (7)
.github/workflows/build.yml (4)
87-165: Deduplicate the five storage jobs with a matrix; add per-job timeout and clearer logs.This cuts ~80% YAML duplication, simplifies maintenance, and makes it trivial to add/remove backends.
jobs: + # Reusable matrix for storage backends + test_storage_backends: + needs: check_format + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + include: + - name: leveldb + module: jraft-extension/leveldb-log-storage-impl + java: 17 + - name: h2mvstore + module: jraft-extension/h2mvstore-log-storage-impl + java: 17 + - name: mapdb + module: jraft-extension/mapdb-log-storage-impl + java: 17 + - name: chroniclemap + module: jraft-extension/chronicle-map-log-storage-impl + java: 8 + - name: bdb + module: jraft-extension/bdb-log-storage-impl + java: 17 + name: "storage:${{ matrix.name }} (JDK ${{ matrix.java }})" + timeout-minutes: 40 + steps: + - uses: actions/checkout@v5 + - name: Set up JDK + uses: actions/setup-java@v5 + with: + distribution: temurin + java-version: ${{ matrix.java }} + cache: maven + - name: Test ${{ matrix.module }} + run: mvn -pl ${{ matrix.module }} -am -Dmaven.javadoc.skip=true -B -V test
3-8: Harden workflow: set minimal permissions and cancel superseded runs.Small top-level additions improve security and CI throughput on force-pushes.
on: push: branches: [ master ] pull_request: branches: [ master ] + +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true
139-145: Chronicle Map job on JDK 8: keep JDK 8, but also add a JDK 17 lane (to reproduce/fix).Given the PR objective (pass on JDK17+), add a second lane for the same module on JDK 17 so failures surface early in CI.
If you don’t move to the matrix refactor above, add a sibling job:
+ test_chroniclemap_log_storage_impl_jdk17: + needs: check_format + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v5 + - name: Set up JDK 17 + uses: actions/setup-java@v5 + with: + distribution: temurin + java-version: 17 + cache: maven + - name: Maven Test + run: mvn -pl jraft-extension/chronicle-map-log-storage-impl -am -Dmaven.javadoc.skip=true -B -V test
86-86: Trim trailing spaces flagged by yamllint.Cosmetic, but keeps CI linters green.
- + ... - || mvn --projects jraft-extension/bdb-log-storage-impl test) + || mvn --projects jraft-extension/bdb-log-storage-impl test)Also applies to: 165-165
.github/workflows/build-macos.yml (3)
1-8: Add minimal permissions and cancel in-progress builds on push updates.Improves security and CI throughput on macOS too.
name: build-macos on: push: branches: [ master ] pull_request: branches: [ master ] + +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true
135-145: Chronicle Map JDK 8 on macOS: consider adding a JDK 17 lane, and pin runner if JDK 8 aarch64 is flaky.
- Add a second macOS job for the same module on JDK 17 to surface incompatibilities early.
- If Temurin 8 on macOS-latest (arm64) becomes unavailable/flaky, pin runner to macos-13 and set architecture: x64.
+ test_chroniclemap_log_storage_impl_jdk17: + needs: check_format + runs-on: macos-latest + steps: + - uses: actions/checkout@v5 + - name: Set up JDK 17 + uses: actions/setup-java@v5 + with: + distribution: temurin + java-version: 17 + cache: maven + - name: Maven Test + run: mvn -pl jraft-extension/chronicle-map-log-storage-impl -am -Dmaven.javadoc.skip=true -B -V testIf JDK 8 fails on arm64, try:
runs-on: macos-13 # and/or with: architecture: x64
86-86: Trim trailing whitespace flagged by yamllint.- +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/build-macos.yml(1 hunks).github/workflows/build-windows.yml(1 hunks).github/workflows/build.yml(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build-windows.yml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/build.yml
[error] 86-86: trailing spaces
(trailing-spaces)
[error] 165-165: trailing spaces
(trailing-spaces)
.github/workflows/build-macos.yml
[error] 86-86: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: sca
🔇 Additional comments (2)
.github/workflows/build.yml (1)
13-16: actions v5 are correct; note Runner minimum (Node 24).actions/checkout@v5 and setup-java@v5 require Actions Runner >= v2.327.1 (hosted runners are already newer). If you have any self-hosted runners, ensure they’re updated.
Reference: checkout v5 and setup-java v5 releases. (github.com, newreleases.io)
Also applies to: 27-30, 43-46, 59-62, 75-78, 91-94, 107-110, 123-126, 139-142, 155-158
.github/workflows/build-macos.yml (1)
13-16: actions v5 OK; ensure Runner is new enough (Node 24).Hosted runners already satisfy the minimum version; update any self-hosted ones.
References: checkout v5, setup-java v5 releases. (github.com)
Also applies to: 27-30, 43-46, 59-62, 75-78, 91-94, 107-110, 123-126, 139-142, 155-158
| - uses: actions/checkout@v5 | ||
| - name: Set up JDK 17 | ||
| uses: actions/setup-java@v5 | ||
| with: | ||
| distribution: temurin | ||
| java-version: 17 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enable Maven cache on macOS jobs too.
Same rationale as Linux: add cache: maven to all setup-java steps.
- name: Set up JDK 17
- uses: actions/setup-java@v5
+ uses: actions/setup-java@v5
with:
distribution: temurin
java-version: 17
+ cache: mavenAlso applies to: 27-32, 43-48, 59-64, 75-80, 91-96, 107-112, 123-128, 139-144, 155-160
🤖 Prompt for AI Agents
.github/workflows/build-macos.yml around lines 13-18: the actions/setup-java@v5
step is missing the Maven cache option; add "cache: maven" under the with: block
for that step, and repeat the same change for the other setup-java occurrences
referenced (lines 27-32, 43-48, 59-64, 75-80, 91-96, 107-112, 123-128, 139-144,
155-160) so every setup-java invocation includes with: distribution: temurin,
java-version: 17 and cache: maven.
| run: mvn clean install -DskipTests=true -Dmaven.javadoc.skip=true -B -V | ||
| && (mvn --projects jraft-core test | ||
| || mvn --projects jraft-core test | ||
| || mvn --projects jraft-core test) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Switch from chained “||” retries to a proper retry with backoff; also build only required modules.
Mirror the Linux suggestions: use a retry step and -pl/-am to avoid building the whole repo per job.
- - name: Maven Test
- run: mvn clean install -DskipTests=true -Dmaven.javadoc.skip=true -B -V
- && (mvn --projects jraft-core test
- || mvn --projects jraft-core test
- || mvn --projects jraft-core test)
+ - name: Maven Test (targeted + retry)
+ shell: bash
+ run: |
+ for i in 1 2 3; do
+ mvn -pl jraft-core -am -Dmaven.javadoc.skip=true -B -V test && break
+ echo "Retry #$i failed; sleeping..." && sleep $((i*10))
+ doneApply for each module section accordingly.
Also applies to: 50-53, 66-69, 82-85, 98-101, 114-117, 130-133, 146-149, 162-165
| - uses: actions/checkout@v5 | ||
| - name: Set up JDK 17 | ||
| uses: actions/setup-java@v3 | ||
| uses: actions/setup-java@v5 | ||
| with: | ||
| distribution: temurin | ||
| java-version: 17 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add Maven cache to all Java setup steps for 2-3x faster CI.
actions/setup-java v5 supports built-in caching. Enable it in every job to cut build time and bandwidth.
- name: Set up JDK 17
- uses: actions/setup-java@v5
+ uses: actions/setup-java@v5
with:
distribution: temurin
java-version: 17
+ cache: mavenApply the same addition (cache: maven) to all setup-java steps listed in this comment’s line ranges.
Reference: setup-java v5 caching notes. (github.com)
Also applies to: 27-33, 43-49, 59-65, 75-81, 91-97, 107-113, 123-129, 139-145, 155-161
🤖 Prompt for AI Agents
.github/workflows/build.yml around lines 13-18: the actions/setup-java@v5 step
is missing the built-in Maven cache option; update that step to include "cache:
maven" in its with block so the job uses setup-java's caching. Apply the same
change to all other setup-java steps referenced (lines 27-33, 43-49, 59-65,
75-81, 91-97, 107-113, 123-129, 139-145, 155-161) by adding cache: maven to each
with section.
1 participant
Motivation:
由于Nacos引用了sofa-jraft,在申威平台上运行时发现RocksDB无法执行，在参考了BDBLogStorage， 增加了基于ChronicleMap、MapDB、H2MVStore、LevelDB-Java实现的日志存储
Modification:
1、在AI（qwen3-coder）的帮助下，参考BDBLogStorage 基于ChronicleMap、MapDB、H2MVStore、LevelDB-Java实现
2、通过单元测试
Result:
目前几个实现都动同过了测试，但是ChronicleMapLogStorage只能在jdk8上通过测试（需要帮助，希望能在jdk17以上版本测试通过）
以下是写入性能
Ubuntu
ChronicleMapLogStorage
Write 100000 logs, cost 4472 ms.
MapDBLogStorage
Write 100000 logs, cost 27631 ms.
H2MVStoreLogStorage
Write 100000 logs, cost 4330 ms.
LevelDBLogStorage
Write 100000 logs, cost 7730 ms.
MacOS
ChronicleMapLogStorage
（github Action MacOS上没有JDK8））
MapDBLogStorage
Write 100000 logs, cost 20340 ms.
H2MVStoreLogStorage
Write 100000 logs, cost 3403 ms.
LevelDBLogStorage
Write 100000 logs, cost 5077 ms.
Windows
ChronicleMapLogStorage
Write 100000 logs, cost 4054 ms.
MapDBLogStorage
Write 100000 logs, cost 226473 ms.
H2MVStoreLogStorage
Write 100000 logs, cost 3295 ms.
LevelDBLogStorage
Write 100000 logs, cost 39628 ms.
Summary by CodeRabbit
New Features
Tests
Chores