contrueCT commented on code in PR #3153:
URL: https://github.com/apache/hugegraph/pull/3153#discussion_r3852471440
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/BusinessHandlerImpl.java:
##########
@@ -1642,7 +1642,7 @@ public TxBuilder merge(int code, String table, byte[]
key, byte[] value) throws
HgStoreException {
try {
- byte[] targetKey = keyCreator.getKey(this.partId, graph, code,
key);
+ byte[] targetKey = keyCreator.getKeyOrCreate(this.partId,
graph, code, key);
Review Comment:
I kept GraphId allocation durable in this focused change. getGraphIdOrCreate
already has durable mapping semantics on the non-batch doPut path, and deleting
a newly created mapping from one batch rollback is unsafe when another
concurrent batch for the same graph may already have observed that ID. Repeated
failures for the same graph reuse its mapping; distinct failed graph names can
still consume mappings, matching the existing non-batch lifecycle. A
transactional or reference-counted GraphId reservation needs a separate
cross-path lifecycle change rather than a rollback-only deletion here.
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/BusinessHandlerImpl.java:
##########
@@ -1577,7 +1577,7 @@ private TxBuilderImpl(String graph, int partId,
RocksDBSession dbSession) {
public TxBuilder put(int code, String table, byte[] key, byte[] value)
throws
HgStoreException {
try {
- byte[] targetKey = keyCreator.getKey(this.partId, graph, code,
key);
+ byte[] targetKey = keyCreator.getKeyOrCreate(this.partId,
graph, code, key);
Review Comment:
Fixed in 2f2765f8. All three TxBuilder call sites now create one Tx before
queuing entries and roll it back on any put, merge, or commit failure; rollback
failures are attached as suppressed exceptions so the original failure is
preserved. TxBuilder rollback closes the RocksDB session and releases the graph
lifecycle lock. BatchGraphIsolationTest now covers the failed DataManagerImpl
and DefaultDataMover write paths.
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/BusinessHandlerImpl.java:
##########
@@ -1577,7 +1577,7 @@ private TxBuilderImpl(String graph, int partId,
RocksDBSession dbSession) {
public TxBuilder put(int code, String table, byte[] key, byte[] value)
throws
HgStoreException {
try {
- byte[] targetKey = keyCreator.getKey(this.partId, graph, code,
key);
+ byte[] targetKey = keyCreator.getKeyOrCreate(this.partId,
graph, code, key);
Review Comment:
Fixed in 2f2765f8 with a striped graph-and-partition lifecycle read/write
lock. TxBuilder holds the read lock from prepare through commit or rollback,
while truncate holds the write lock across both deleteRange and delGraphId. The
new deterministic regression test starts truncate after a batch has queued an
encoded key, verifies truncate remains blocked, then commits and verifies
truncate can finish before the next graph is written.
##########
hugegraph-store/hg-store-test/src/main/java/org/apache/hugegraph/store/core/BatchGraphIsolationTest.java:
##########
@@ -0,0 +1,186 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hugegraph.store.core;
+
+import static org.apache.hugegraph.store.constant.HugeServerTables.TABLES_MAP;
+import static
org.apache.hugegraph.store.constant.HugeServerTables.VERTEX_TABLE;
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.hugegraph.store.UnitTestBase;
+import org.apache.hugegraph.store.business.BusinessHandler;
+import org.apache.hugegraph.store.business.BusinessHandlerImpl;
+import org.apache.hugegraph.store.grpc.common.Key;
+import org.apache.hugegraph.store.grpc.common.OpType;
+import org.apache.hugegraph.store.grpc.session.BatchEntry;
+import org.apache.hugegraph.store.meta.PartitionManager;
+import org.apache.hugegraph.store.options.HgStoreEngineOptions;
+import org.apache.hugegraph.store.options.RaftRocksdbOptions;
+import org.apache.hugegraph.store.pd.FakePdServiceProvider;
+import org.apache.hugegraph.store.pd.PdProvider;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import com.alipay.sofa.jraft.util.StorageOptionsFactory;
+import com.google.protobuf.ByteString;
+
+public class BatchGraphIsolationTest {
+
+ private static final int PARTITION_ID = 0;
+ private static final int EMPTY_PARTITION_ID = 1;
+ private static final int KEY_CODE = 0;
+ private static final byte[] SHARED_KEY =
+ "shared-key".getBytes(StandardCharsets.UTF_8);
+
+ private static Path databasePath;
+ private static BusinessHandler handler;
+
+ @BeforeClass
+ public static void setup() throws IOException {
+ databasePath =
Files.createTempDirectory("hugegraph-batch-graph-isolation-");
+
+ Map<String, Object> rocksdbConfig = new HashMap<>();
+ rocksdbConfig.put("rocksdb.write_buffer_size", "1048576");
+ StorageOptionsFactory.releaseAllOptions();
+ RaftRocksdbOptions.initRocksdbGlobalConfig(rocksdbConfig);
+ BusinessHandlerImpl.initRocksdb(rocksdbConfig, null);
+
+ HgStoreEngineOptions options = new HgStoreEngineOptions();
+ options.setDataPath(databasePath.toString());
+ options.setRaftPath(databasePath.toString());
+
+ HgStoreEngineOptions.FakePdOptions fakePdOptions =
+ new HgStoreEngineOptions.FakePdOptions();
+ fakePdOptions.setPartitionCount(1);
Review Comment:
Fixed in 2f2765f8. The fake PD now configures two valid partitions and
hasPartition accepts partition 1. The regression test sends a real doBatch PUT
with code 32768 to partition 1 while g+v is absent, uses a 5-second JUnit
timeout, and reads the value back through the normal graph path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
