Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion .github/workflows/pd-store-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,11 @@ jobs:
mvn test -pl hugegraph-store/hg-store-test -am \
-P store-raftcore-test -Djacoco.sessionId=store-raftcore-test

- name: Run core test
run: |
mvn test -pl hugegraph-store/hg-store-test -am \
-P store-core-test -Djacoco.sessionId=store-core-test

- name: Generate aggregate coverage report
run: |
mvn verify -pl hugegraph-store/hg-store-test -am -P jacoco \
Expand All @@ -311,15 +316,19 @@ jobs:
"$TEST_REPORT_DIR/TEST-org.apache.hugegraph.store.rocksdb.RocksDbSuiteTest.xml" \
--require-test-report \
"$TEST_REPORT_DIR/TEST-org.apache.hugegraph.store.raftcore.RaftSuiteTest.xml" \
--require-test-report \
"$TEST_REPORT_DIR/TEST-org.apache.hugegraph.store.core.CoreSuiteTest.xml" \
--require-covered-group hg-store-common \
--require-covered-group hg-store-client \
--require-covered-group hg-store-rocksdb \
--require-covered-group hg-store-core \
--require-session store-common-test \
--require-session store-client-test \
--require-session store-rocksdb-test \
--require-session store-raftcore-test \
--require-session store-core-test \
"$REPORT_FILE" \
hg-store-grpc hg-store-common hg-store-client hg-store-rocksdb
hg-store-grpc hg-store-common hg-store-client hg-store-rocksdb hg-store-core

- name: Upload coverage to Codecov
uses: codecov/codecov-action@v5
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -499,21 +499,22 @@ assert "mvn verify -pl hugegraph-store/hg-store-test -am -P jacoco \\ " \
"-DskipTests -Deditorconfig.skip=true -ntp" in " ".join(store_job.split())
assert selected_profiles(store_job, "store") == {
"store-common-test", "store-client-test", "store-rocksdb-test",
"store-raftcore-test",
"store-raftcore-test", "store-core-test",
}
assert reports_for_option(store_job, "--require-test-report") == {
"TEST-org.apache.hugegraph.store.common.CommonSuiteTest.xml",
"TEST-org.apache.hugegraph.store.client.ClientSuiteTest.xml",
"TEST-org.apache.hugegraph.store.rocksdb.RocksDbSuiteTest.xml",
"TEST-org.apache.hugegraph.store.raftcore.RaftSuiteTest.xml",
"TEST-org.apache.hugegraph.store.core.CoreSuiteTest.xml",
}
assert not reports_for_option(store_job, "--require-suite-report")
assert values_for_option(store_job, "--require-covered-group") == {
"hg-store-common", "hg-store-client", "hg-store-rocksdb",
"hg-store-common", "hg-store-client", "hg-store-rocksdb", "hg-store-core",
}
assert required_modules(store_job) == {
"hg-store-grpc", "hg-store-common", "hg-store-client",
"hg-store-rocksdb",
"hg-store-rocksdb", "hg-store-core",
}

print("PASS: JaCoCo aggregation configuration contract")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,14 @@ void lock(String path) throws InterruptedException,

void unlock(String path);

/**
* Non-blocking attempt to reserve the compactRange() window for partition {@code id}.
* Returns false if a compaction is actively running for that partition right now.
*/
boolean tryLockCompactionRange(int id);

void unlockCompactionRange(int id);

void awaitAndSetLock(int id, int expectedValue, int value) throws InterruptedException,
TimeoutException;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReadWriteLock;
import java.util.concurrent.locks.ReentrantLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.function.BiFunction;
import java.util.function.Consumer;
Expand Down Expand Up @@ -138,6 +139,12 @@ public class BusinessHandlerImpl implements BusinessHandler {
private static final ConcurrentMap<String, AtomicInteger> pathLock = new ConcurrentHashMap<>();
private static final ConcurrentMap<Integer, AtomicInteger> compactionState =
new ConcurrentHashMap<>();
// Guards the compactRange() window specifically, so a snapshot save can atomically
// check-and-reserve against a compaction that is actually running right now. This is
// narrower than pathLock, which stays held through the post-compaction blank-task
// snapshot and must not be reused here to avoid deadlocking that flow.
private static final ConcurrentMap<Integer, ReentrantLock> compactionRangeLock =
new ConcurrentHashMap<>();
// Default core thread count
private static final int compactionThreadCount = 64;
private static final int compactionMaxThreadCount = 256;
Expand Down Expand Up @@ -1415,10 +1422,27 @@ public boolean dbCompaction(String graphName, int id, String tableName) {
log.info("Partition {} dbCompaction started", id);
if (tableName.isEmpty()) {
lock(path);
setState(id, doing);
log.info("Partition {}-{} got lock, dbCompaction start", id, path);
op.compactRange();
setState(id, compactionDone);
ReentrantLock rangeLock =
compactionRangeLock.computeIfAbsent(id,
k -> new ReentrantLock());
if (!rangeLock.tryLock()) {
// A snapshot save is currently reserving this partition's
// range lock. Skip this compaction pass rather than block
// the compactionPool thread on it - the next scheduled/
// triggered compaction will retry.
log.info("Partition {} skip dbCompaction, snapshot save in " +
"progress", id);
unlock(path);
return;
}
try {
setState(id, doing);
log.info("Partition {}-{} got lock, dbCompaction start", id, path);
op.compactRange();
setState(id, compactionDone);
} finally {
rangeLock.unlock();
}
log.info("Partition {} dbCompaction end and start to do snapshot", id);
PartitionEngine pe = HgStoreEngine.getInstance().getPartitionEngine(id);
// find leader and send blankTask, after execution
Expand Down Expand Up @@ -1484,6 +1508,20 @@ private boolean compareAndSetLock(String path) {
return l.compareAndSet(compactionCanStart, doing);
}

@Override
public boolean tryLockCompactionRange(int id) {
ReentrantLock rangeLock = compactionRangeLock.computeIfAbsent(id, k -> new ReentrantLock());
return rangeLock.tryLock();
}

@Override
public void unlockCompactionRange(int id) {
ReentrantLock rangeLock = compactionRangeLock.get(id);
if (rangeLock != null) {
rangeLock.unlock();
}
}

@Override
public void awaitAndSetLock(int id, int expectedValue, int value) throws InterruptedException,
TimeoutException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
public class RaftRocksdbOptions {

private static RocksdbConfig rocksdbConfig = null;
private static boolean raftRocksdbConfigRegistered = false;

private static RocksdbConfig getRocksdbConfig(HugeConfig options) {
if (rocksdbConfig == null) {
Expand All @@ -55,6 +56,15 @@ private static RocksdbConfig getRocksdbConfig(HugeConfig options) {
}

private static void registerRaftRocksdbConfig(HugeConfig options) {
// StorageOptionsFactory.releaseAllOptions() (called by test setup between runs)
// does not clear its table-format-config table, so registering RocksDBLogStorage's
// config more than once per JVM throws IllegalStateException. Register only once.
synchronized (RaftRocksdbOptions.class) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 The guard releases the monitor before the registration it is guarding.

raftRocksdbConfigRegistered flips inside synchronized (RaftRocksdbOptions.class) at line 66, but every StorageOptionsFactory.register* call sits outside the block (lines 68-103, the calls at 81, 90 and 102). Because the flag is set before the work, a throw anywhere in the body, new LRUCache(SizeUnit.GB) on line 68 for instance, permanently suppresses every later attempt in that JVM with no log line, and the process then runs on jraft defaults instead of the configured DBOptions/ColumnFamilyOptions. The same gap lets a concurrent second caller return early while the first is still registering, though today only test setups call this more than once.

Requested change: hold the monitor across the whole method body, or set raftRocksdbConfigRegistered = true only after the final registerRocksDBColumnFamilyOptions call.

if (raftRocksdbConfigRegistered) {
return;
}
raftRocksdbConfigRegistered = true;
}
Cache blockCache = new LRUCache(SizeUnit.GB);
BlockBasedTableConfig tableConfig = new BlockBasedTableConfig()
.setIndexType(IndexType.kTwoLevelIndexSearch)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.zip.Checksum;

import org.apache.commons.io.FileUtils;
Expand Down Expand Up @@ -94,35 +93,41 @@ public void onSnapshotSave(final SnapshotWriter writer) throws HgStoreException
final String snapshotDir = writer.getPath();
if (partitionEngine != null) {
Integer groupId = partitionEngine.getGroupId();
AtomicInteger state = businessHandler.getState(groupId);
if (state != null && state.get() == BusinessHandler.doing) {
return;
if (!businessHandler.tryLockCompactionRange(groupId)) {
throw new HgStoreException(HgStoreException.EC_RKDB_SNAPSHOT_SAVE_BUSY_FAIL,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

‼️ This does more than refuse one snapshot: it restarts the partition's raft node.

PartitionStateMachine.onSnapshotSave (line 199-201, unchanged by this PR) turns the exception into done.run(new Status(RaftError.EIO, ...)), and EIO is the one code jraft escalates. In jraft-core-1.3.13 SnapshotExecutorImpl.onSnapshotSaveDone, any other non-zero result only reaches writer.setError(...), but:

if (ret == RaftError.EIO.getNumber()) {
    reportError(RaftError.EIO.getNumber(), "Fail to save snapshot.");
}

reportError goes to FSMCallerImpl.setError, which calls fsm.onError and node.onError. On this side PartitionStateMachine.java:129-134 forwards to the state listeners and PartitionEngine.java:709-712 implements onError as restartRaftNode(), i.e. shutdown(); init(this.options);. NodeImpl.onError also steps down and sets State.STATE_ERROR.

So a scheduled snapshot landing inside a compactRange() now costs a teardown and re-init of that partition's raft node: leader step-down, re-election, log storage close/reopen, replay. Shipped defaults make that overlap realistic, snapshotInterval: 1800 in hg-store-node/src/main/resources/application.yml and hg-store-dist/src/assembly/static/conf/application.yml, 300 as the @Value fallback in AppConfig.java:183, against a full RocksDB range compaction on a large partition.

Requested change: report busy with a code jraft does not escalate, RaftError.EBUSY (1009) instead of EIO (1014). With any non-EIO code, onSnapshotSaveDone sets the writer error and LocalSnapshotStorage.close destroys the temp snapshot directory without reportError, so the incomplete snapshot is still refused (the #3162 fix holds) while the node keeps serving and jraft retries next interval. That needs PartitionStateMachine.onSnapshotSave to catch EC_RKDB_SNAPSHOT_SAVE_BUSY_FAIL separately from a real save failure; alternatively, wait a bounded interval for the lock here before giving up.

String.format(
"Partition %d snapshot save failed: " +
"compaction in progress", groupId));
}
// rocks db snapshot
final String graphSnapshotDir = snapshotDir + File.separator + SNAPSHOT_DATA_PATH;
businessHandler.saveSnapshot(graphSnapshotDir, "", groupId);

List<String> files = new ArrayList<>();
File dir = new File(graphSnapshotDir);
File rootDirFile = new File(writer.getPath());
// add all files in data dir
findFileList(dir, rootDirFile, files);

// load snapshot by learner ??
for (String file : files) {
String checksum = calculateChecksum(writer.getPath() + File.separator + file);
if (checksum.length() != 0) {
LocalFileMetaOutter.LocalFileMeta meta =
LocalFileMetaOutter.LocalFileMeta.newBuilder()
.setChecksum(checksum)
.build();
writer.addFile(file, meta);
} else {
writer.addFile(file);
try {
// rocks db snapshot
final String graphSnapshotDir = snapshotDir + File.separator + SNAPSHOT_DATA_PATH;
businessHandler.saveSnapshot(graphSnapshotDir, "", groupId);

List<String> files = new ArrayList<>();
File dir = new File(graphSnapshotDir);
File rootDirFile = new File(writer.getPath());
// add all files in data dir
findFileList(dir, rootDirFile, files);

// load snapshot by learner ??
for (String file : files) {
String checksum = calculateChecksum(writer.getPath() + File.separator + file);
if (checksum.length() != 0) {
LocalFileMetaOutter.LocalFileMeta meta =
LocalFileMetaOutter.LocalFileMeta.newBuilder()
.setChecksum(checksum)
.build();
writer.addFile(file, meta);
} else {
writer.addFile(file);
}
}
// should_not_load wound not sync to learner
markShouldNotLoad(writer, true);
} finally {
businessHandler.unlockCompactionRange(groupId);
}
// should_not_load wound not sync to learner
markShouldNotLoad(writer, true);
}
}

Expand Down Expand Up @@ -169,6 +174,15 @@ private String calculateChecksum(String path) {
public void onSnapshotLoad(final SnapshotReader reader, long committedIndex) throws
HgStoreException {
final String snapshotDir = reader.getPath();
final String graphSnapshotDir = snapshotDir + File.separator + SNAPSHOT_DATA_PATH;

if (!new File(graphSnapshotDir).isDirectory()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Placing this above the should_not_load early return widens it past the corruption it targets.

Line 188 returns early for a locally saved snapshot precisely because nothing is loaded from it. With the check above that return, such a snapshot missing data/ now throws, PartitionStateMachine.onSnapshotLoad (line 228-235) returns false, FSMCallerImpl.doSnapshotLoad calls setError(ESTATEMACHINE), and that reaches PartitionEngine.onError -> restartRaftNode(). Re-init reads the same on-disk snapshot, so it repeats.

The #3162 signature does not need the wider placement: the old early return in onSnapshotSave happened before markShouldNotLoad, so a snapshot corrupted that way has no flag, shouldNotLoad is false, and the check still fires from below the return.

Requested change: move the block after the shouldNotLoad(reader) early return at lines 187-191. testOnSnapshotLoadThrowsWhenShouldNotLoadPresentButDataMissing then needs its expectation flipped to "skips", which is the behaviour before this PR.

throw new HgStoreException(HgStoreException.EC_RKDB_IMPORT_SNAPSHOT_FAIL,
String.format(
"Raft %d snapshot is corrupt, data dir %s is " +
"missing", partitionEngine.getGroupId(),
graphSnapshotDir));
}

// No need to load locally saved snapshots
if (shouldNotLoad(reader)) {
Expand All @@ -177,7 +191,6 @@ public void onSnapshotLoad(final SnapshotReader reader, long committedIndex) thr
}

// Use snapshot directly
final String graphSnapshotDir = snapshotDir + File.separator + SNAPSHOT_DATA_PATH;
log.info("Raft {} begin loadSnapshot, {}", partitionEngine.getGroupId(), graphSnapshotDir);
businessHandler.loadSnapshot(graphSnapshotDir, "", partitionEngine.getGroupId(),
committedIndex);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,9 @@ public class HgStoreException extends RuntimeException {
public static final int EC_RKDB_DOMERGE_FAIL = 1207;
public static final int EC_RKDB_DOGET_FAIL = 1208;
public static final int EC_RKDB_PD_FAIL = 1209;
public static final int EC_RKDB_TRUNCATE_FAIL = 1212;
public static final int EC_RKDB_EXPORT_SNAPSHOT_FAIL = 1214;
public static final int EC_RKDB_IMPORT_SNAPSHOT_FAIL = 1215;
public static final int EC_RKDB_TRANSFER_SNAPSHOT_FAIL = 1216;
public static final int EC_METRIC_FAIL = 1401;
public static final int EC_RKDB_SNAPSHOT_SAVE_BUSY_FAIL = 1217;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 This hunk also deletes three public constants, which is unrelated to the snapshot fix.

Alongside adding EC_RKDB_SNAPSHOT_SAVE_BUSY_FAIL = 1217, the diff removes EC_RKDB_TRUNCATE_FAIL = 1212, EC_RKDB_TRANSFER_SNAPSHOT_FAIL = 1216 and EC_METRIC_FAIL = 1401. Grepping the head tree for the three names returns nothing, so nothing in-repo breaks, but they are public static final members of a type published in the hg-store-core artifact: anything downstream that recompiles against the new jar stops compiling. Nothing in this PR needs the removal.

Requested change: restore the three constants and keep this hunk to the single added code. If the cleanup is wanted, it belongs in its own PR.

private static final long serialVersionUID = 5193624480997934335L;
private final int code;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@
import org.junit.Test;
import org.mockito.Mockito;

import com.alipay.sofa.jraft.util.StorageOptionsFactory;
import com.google.protobuf.ByteString;

public class BatchGraphIsolationTest {
Expand All @@ -78,7 +77,6 @@ public static void setup() throws IOException {

Map<String, Object> rocksdbConfig = new HashMap<>();
rocksdbConfig.put("rocksdb.write_buffer_size", "1048576");
StorageOptionsFactory.releaseAllOptions();
RaftRocksdbOptions.initRocksdbGlobalConfig(rocksdbConfig);
BusinessHandlerImpl.initRocksdb(rocksdbConfig, null);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,14 @@

package org.apache.hugegraph.store.core;

import org.apache.hugegraph.store.core.snapshot.HgSnapshotHandlerTest;
import org.junit.runner.RunWith;
import org.junit.runners.Suite;

import lombok.extern.slf4j.Slf4j;

// TODO: uncomment it until all test can run free.
//@RunWith(Suite.class)
//@Suite.SuiteClasses({
// TODO: uncomment the rest of these classes once they can run free of each other.
// HgCmdClientTest.class,
// HgSnapshotHandlerTest.class,
// RaftUtilsTest.class,
// RaftOperationTest.class,
// UnsafeUtilTest.class,
Expand All @@ -41,8 +42,10 @@
// PartitionInstructionProcessorTest.class,
// // Try to put it last
// HgBusinessImplTest.class
//})

@RunWith(Suite.class)
@Suite.SuiteClasses({
HgSnapshotHandlerTest.class

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Re-enabling this suite makes the Run core test step added in this PR fail, and it is red on this head.

The store-core-test surefire execution runs **/CoreSuiteTest.java and **/BatchGraphIsolationTest.java in one fork (hugegraph-store/hg-store-test/pom.xml:245-248). With HgSnapshotHandlerTest back in the suite, CoreSuiteTest pulls in StoreEngineTestBase, whose @AfterClass shutDownEngine() calls HgStoreEngine.getInstance().shutdown(). That singleton's closing flag is never cleared, so the next class in the same JVM cannot open a session and BusinessHandlerImpl.getSession throws at BusinessHandlerImpl.java:1318.

From job 101289530213, step 16:

10:20:50.494 [INFO]  Tests run: 7 ... in org.apache.hugegraph.store.core.CoreSuiteTest
10:20:50.505 [ERROR] HgStoreException: store is closing
                       at BatchGraphIsolationTest.setup(BatchGraphIsolationTest.java:113)
[ERROR] Failed to execute goal ... maven-surefire-plugin:2.20:test (store-core-test)

This is what the deleted // TODO: uncomment it until all test can run free. was warning about.

Requested change: stop the two sharing a JVM. Move **/BatchGraphIsolationTest.java into its own surefire execution, or set <reuseForks>false</reuseForks> on store-core-test, so the engine CoreSuiteTest shuts down cannot leak into it.

})
@Slf4j
public class CoreSuiteTest {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,12 @@
import org.apache.hugegraph.store.meta.Partition;
import org.apache.hugegraph.store.meta.ShardGroup;
import org.apache.hugegraph.store.options.HgStoreEngineOptions;
import org.apache.hugegraph.store.options.JobOptions;
import org.apache.hugegraph.store.options.RaftRocksdbOptions;
import org.apache.hugegraph.store.pd.FakePdServiceProvider;
import org.junit.AfterClass;
import org.junit.BeforeClass;

import com.alipay.sofa.jraft.util.StorageOptionsFactory;

import lombok.extern.slf4j.Slf4j;

/**
Expand All @@ -61,6 +60,11 @@ public static void initEngine() {
options.setGrpcAddress("127.0.0.1:6511");
options.setRaftAddress("127.0.0.1:6510");
options.setDataTransfer(new DataManagerImpl());
JobOptions jobOptions = new JobOptions();
jobOptions.setUninterruptibleCore(2);
jobOptions.setUninterruptibleMax(8);
jobOptions.setUninterruptibleQueueSize(1024);
options.setJobConfig(jobOptions);

options.setFakePdOptions(new HgStoreEngineOptions.FakePdOptions() {{
setStoreList("127.0.0.1");
Expand All @@ -70,7 +74,6 @@ public static void initEngine() {
}});

if (initCount == 0) {
StorageOptionsFactory.releaseAllOptions();
RaftRocksdbOptions.initRocksdbGlobalConfig(options.getRocksdbConfig());
initCount++;
}
Expand Down
Loading
Loading