bitflicker64 commented on code in PR #3299:
URL: https://github.com/apache/hugegraph/pull/3299#discussion_r4257615305
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/rocksdb/RocksDBSessionsTest.java:
##########
@@ -308,6 +310,88 @@ protected Map<String, RocksDBSessions> tableDBMapping() {
}
}
+ @Test
+ public void testToplingOlapClearKeepsHandleAndOtherTables() throws
Exception {
+ this.assertOlapClear(true, false);
+ }
+
+ @Test
+ public void testToplingOlapClearRoutesIndependentDatabase() throws
Exception {
+ this.assertOlapClear(true, true);
+ }
+
+ @Test
+ public void testStandardOlapClearRecreatesTable() throws Exception {
+ this.assertOlapClear(false, false);
+ }
+
+ private void assertOlapClear(boolean topling, boolean separate) throws
Exception {
+ Id id = IdGenerator.of(123L);
+ String path = DB_PATH + "/olap-clear-" + topling + "-" + separate;
+ RocksDBSessions olap = new RocksDBStdSessions(FakeObjects.newConfig(),
"db", "olap", path, path);
+ RocksDBStore.RocksDBGraphStore store = new
RocksDBStore.RocksDBGraphStore(
+ new RocksDBStoreProvider(), "db", "store") {
+ @Override
+ protected RocksDBSessions db(HugeType type) {
+ return type == HugeType.OLAP ? olap : super.db(type);
+ }
+ };
+ Whitebox.setInternalState(store, "sessions", separate ? this.rocks :
olap);
+ Whitebox.setInternalState(store, "toplingProvider", topling);
+ try {
+ olap.session();
+ store.createOlapTable(id);
+ String table = "store+ap_123";
+ olap.createTable(TABLE, "other-olap");
+ RocksDBSessions.Session session = olap.session();
+ session.put(table, getBytes("m"), getBytes("old"));
+ session.put(table, getBytes("n"), getBytes("old"));
+ session.put("other-olap", getBytes("other"), getBytes("retained"));
+ session.commit();
+ session.put(TABLE, getBytes("main"), getBytes("retained main"));
+ session.commit();
+ AtomicReference<?> owner = Whitebox.getInternalState(olap,
"rocksdb");
+ Object opened = owner.get();
+ Map<String, ?> handles = Whitebox.getInternalState(opened,
"cfHandles");
+ Object handle = handles.get(table);
+ Assert.assertNotNull(handle);
+ for (int i = 0; i < 2; i++) {
+ // Pending keys lie outside the stored range, or in an empty
table.
+ if (topling) {
+ session.put(table, getBytes("a"), getBytes("pending
first"));
+ session.put(table, getBytes("z"), getBytes("pending
last"));
+ }
+ store.clearOlapTable(id);
+ if (topling) {
+ Assert.assertSame(handle, handles.get(table));
+ } else {
+ Assert.assertNotSame(handle, handles.get(table));
Review Comment:
Minor: on the standard branch this assertion only proves recreation for the
first clear. `handle` is read once before the loop (line 356) and never
refreshed, so on the second iteration it is still the pre-first-clear handle
and `assertNotSame` passes even if the second `clearOlapTable` did not drop and
recreate the table. Please re-read the handle at the end of each iteration (for
example `handle = handles.get(table)` after the assertions on the standard
branch) so the repeated clear is actually checked.
##########
hugegraph-server/hugegraph-rocksdb/src/main/java/org/apache/hugegraph/backend/store/rocksdb/RocksDBStore.java:
##########
@@ -1135,14 +1135,32 @@ public void createOlapTable(Id id) {
@Override
public void clearOlapTable(Id id) {
- String name = this.olapTableName(id);
- RocksDBTable table = this.table(name);
- RocksDBSessions db = this.db(HugeType.OLAP);
- if (!db.existsTable(table.table())) {
- throw new HugeException("Not exist table '%s''", name);
+ Lock writeLock = this.storeLock().writeLock();
+ writeLock.lock();
+ try {
+ String name = this.olapTableName(id);
+ RocksDBTable table = this.table(name);
+ RocksDBSessions db = this.db(HugeType.OLAP);
+ if (!db.existsTable(table.table())) {
+ throw new HugeException("Not exist table '%s''", name);
+ }
+ if (super.toplingProvider) {
+ // Keep the CF owner alive. Use only the clearing thread's
+ // lease so pending writes cannot be committed by
clearTables.
+ RocksDBSessions.Session session =
(RocksDBSessions.Session) db.useSession();
+ try {
+ session.reset();
Review Comment:
Minor: the reset is limited to the clearing thread's session, and the PR
description states that other threads' leases are left untouched, but
`assertOlapClear` only writes pending data from the same thread that calls
`clearOlapTable`. Nothing pins what happens to a batch another thread has
already filled for this table: `RocksDBSessions.clearTables` only range-deletes
committed keys, so that batch commits after the write lock is released and its
rows survive the clear, unlike `truncate()`, which calls `forceResetSessions`
on every database. Please add a test that puts to the OLAP table from a second
thread, runs the clear, commits on the second thread and asserts the intended
outcome, and state that outcome in `docs/storage-lifecycle.md`.
--
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]
