You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hugegraph.apache.org by "contrueCT (via GitHub)" <gi...@apache.org> on 2026/10/09 12:11:53 UTC
[PR] fix(rocksdb): retain Topling OLAP handles [hugegraph]
contrueCT opened a new pull request, #3299:
URL: https://github.com/apache/hugegraph/pull/3299
## Purpose of the PR
Topling's standalone OLAP property clear still drops and recreates the same column family. The previously reported real-JNI reproduction completes the clear task, then aborts during normal shutdown. This change clears the selected table while retaining its CF handle.
Addresses the OLAP clear/close item already tracked in #3242 and [the original review](https://github.com/hugegraph/hugegraph/pull/266#discussion_r4214095995). The release checklist stays open. Existing Topling and recovery PRs were checked; the merged runtime integration and current #3293 do not cover this clear path.
## Main Changes
- Clear the selected OLAP table in its owning database under the store write lock.
- Retain Topling CF handles and discard the clearing thread's pending batch before committing the clear; leave other threads' leases untouched.
- Preserve the standard RocksDB drop/create path.
## Verifying these changes
- [ ] Trivial rework / code cleanup without any test coverage. (No Need)
- [x] `RocksDBSessionsTest`: 22 tests pass on Java 17, with no failures/errors/skips. New tests cover retained handle identity, repeated clears, pending writes outside the stored range and in an empty table, independent OLAP routing, other-table preservation, reuse and close/reopen reads.
- [x] Affected Server reactor clean compilation, editorconfig formatting and `git diff --check` pass.
These tests run the Topling Java branch with the real standard RocksDB JNI. The same handle-preservation regression fails against the original master adapter. Actual Topling JNI and its reported shutdown abort were not rerun on this macOS arm64 host; current-head Linux native qualification remains necessary.
## Does this PR potentially affect the following parts?
- [ ] Dependencies
- [ ] Modify configurations
- [ ] The public API
- [x] Other affects: standalone Topling OLAP clear and native handle lifecycle
- [ ] Nope
## Documentation Status
- [ ] `Doc - TODO`
- [x] `Doc - Done`: `docs/toplingdb.md` documents OLAP clear, handle retention and pending-write handling.
- [ ] `Doc - No Need`
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(rocksdb): retain Topling OLAP handles [hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(rocksdb): retain Topling OLAP handles [hugegraph]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #3299:
URL: https://github.com/apache/hugegraph/pull/3299#issuecomment-6080954914
## [Codecov](https://app.codecov.io/gh/apache/hugegraph/pull/3299?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `16 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 33.54%. Comparing base ([`6885519`](https://app.codecov.io/gh/apache/hugegraph/commit/68855199031d5801edb4fe41b2bacbacffe8fe68?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`a440bdc`](https://app.codecov.io/gh/apache/hugegraph/commit/a440bdc461d41bfa184e73fd82ecb789c4e1c7b0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/hugegraph/pull/3299?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../hugegraph/backend/store/rocksdb/RocksDBStore.java](https://app.codecov.io/gh/apache/hugegraph/pull/3299?src=pr&el=tree&filepath=hugegraph-server%2Fhugegraph-rocksdb%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhugegraph%2Fbackend%2Fstore%2Frocksdb%2FRocksDBStore.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVnZWdyYXBoLXNlcnZlci9odWdlZ3JhcGgtcm9ja3NkYi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaHVnZWdyYXBoL2JhY2tlbmQvc3RvcmUvcm9ja3NkYi9Sb2Nrc0RCU3RvcmUuamF2YQ==) | 0.00% | [16 Missing :warning: ](https://app.codecov.io/gh/apache/hugegraph/pull/3299?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #3299 +/- ##
============================================
- Coverage 37.05% 33.54% -3.51%
+ Complexity 6006 3775 -2231
============================================
Files 757 492 -265
Lines 65152 45896 -19256
Branches 8604 6500 -2104
============================================
- Hits 24141 15398 -8743
+ Misses 38322 28508 -9814
+ Partials 2689 1990 -699
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/hugegraph/pull/3299?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(rocksdb): retain Topling OLAP handles [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #3299:
URL: https://github.com/apache/hugegraph/pull/3299#discussion_r4231249190
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/rocksdb/RocksDBSessionsTest.java:
##########
@@ -226,6 +228,87 @@ 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();
+ Object opened = Whitebox.getInternalState(olap, "rocksdb");
+ Map<String, ?> handles = Whitebox.getInternalState(opened, "cfHandles");
Review Comment:
Integration note for #3293, not a defect in this head: this fixture reads `rocksdb` as the owner directly. At `2d335c9889e73e08a2c08a077e002d1294b0f601`, #3293 changes that field to `AtomicReference<OpenedRocksDB>` and updates the existing fixtures to call `owner.get()`. When combining these changes, please apply the same dereference here before reading `cfHandles`; otherwise the three new OLAP-clear tests will reflect on `AtomicReference`. This is a source-level integration finding; no combined-tree test run was performed.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org