bitflicker64 commented on code in PR #3306:
URL: https://github.com/apache/hugegraph/pull/3306#discussion_r4236982145
##########
hugegraph-server/hugegraph-hstore/src/main/java/org/apache/hugegraph/backend/store/hstore/HstoreStore.java:
##########
@@ -765,16 +771,68 @@ public void checkAndRegisterOlapTable(Id pkId) {
@Override
public void clearOlapTable(Id pkId) {
+ this.deleteOlapData(pkId);
}
@Override
public void removeOlapTable(Id pkId) {
+ // Every OLAP property shares this table and its registered
manager.
+ this.deleteOlapData(pkId);
+ }
+
+ private void deleteOlapData(Id pkId) {
+ E.checkArgumentNotNull(pkId, "OLAP property ID");
+ Session session = this.session(HugeType.OLAP);
+ String tableName = this.table(HugeType.OLAP).table();
+ E.checkState(!session.hasChanges(), "Can't clear OLAP data with
pending graph mutations");
+ E.checkState(super.sessions.existsTable(tableName), "Not exist
table '%s'", tableName);
+ byte[] propertyPrefix = BytesBuffer.allocate(pkId.length() +
9).writeId(pkId).bytes();
+ int pending = 0;
+ try {
+ // A property prefix alone also matches legacy vertex-only
keys from other properties.
+ try (BackendColumnIterator columns = session.scan(tableName)) {
+ while (columns.hasNext()) {
+ BackendColumn column = columns.next();
+ if (!OlapKey.matchesProperty(column.value, pkId)) {
+ boolean targetCompound = column.name != null &&
+ column.name.length >
propertyPrefix.length &&
+
Bytes.prefixWith(column.name, propertyPrefix);
+ E.checkState(!targetCompound, "OLAP value must
match property '%s'", pkId);
Review Comment:
Minor: one inconsistent row makes both clear and remove fail on every retry,
and the message does not say which row.
Evidence: this check, and the two `E.checkState` calls on the matching
branch below, abort the whole cleanup. `OlapPropertyKeyRemoveJob.execute()`
calls `removeOlapPk()` first, so the schema removal after it never runs and the
property key can no longer be removed. The messages only carry `pkId`, and
there is no API to delete a single raw OLAP row.
Requested change: include the offending key (for example
`Bytes.toHex(column.name)`) in these messages, and say in
`docs/shared-foundation-migration.md` that such a row blocks cleanup until it
is repaired.
##########
hugegraph-server/hugegraph-hstore/src/main/java/org/apache/hugegraph/backend/store/hstore/HstoreStore.java:
##########
@@ -765,16 +771,68 @@ public void checkAndRegisterOlapTable(Id pkId) {
@Override
public void clearOlapTable(Id pkId) {
+ this.deleteOlapData(pkId);
}
@Override
public void removeOlapTable(Id pkId) {
+ // Every OLAP property shares this table and its registered
manager.
+ this.deleteOlapData(pkId);
+ }
+
+ private void deleteOlapData(Id pkId) {
+ E.checkArgumentNotNull(pkId, "OLAP property ID");
+ Session session = this.session(HugeType.OLAP);
+ String tableName = this.table(HugeType.OLAP).table();
+ E.checkState(!session.hasChanges(), "Can't clear OLAP data with
pending graph mutations");
+ E.checkState(super.sessions.existsTable(tableName), "Not exist
table '%s'", tableName);
+ byte[] propertyPrefix = BytesBuffer.allocate(pkId.length() +
9).writeId(pkId).bytes();
+ int pending = 0;
+ try {
+ // A property prefix alone also matches legacy vertex-only
keys from other properties.
+ try (BackendColumnIterator columns = session.scan(tableName)) {
+ while (columns.hasNext()) {
+ BackendColumn column = columns.next();
+ if (!OlapKey.matchesProperty(column.value, pkId)) {
+ boolean targetCompound = column.name != null &&
+ column.name.length >
propertyPrefix.length &&
+
Bytes.prefixWith(column.name, propertyPrefix);
+ E.checkState(!targetCompound, "OLAP value must
match property '%s'", pkId);
+ continue;
+ }
+ BytesBuffer key = BytesBuffer.wrap(column.name);
+ Id first = key.readId();
+ Id vertexId = first;
+ if (key.remaining() != 0) {
+ E.checkState(pkId.equals(first), "OLAP key must
match property '%s'", pkId);
+ vertexId = key.readId();
+ E.checkState(key.remaining() == 0, "Unexpected
trailing bytes in OLAP key");
+ }
+ session.delete(tableName, vertexId.asBytes(),
column.name);
+ if (++pending == OLAP_DELETE_BATCH_SIZE) {
+ session.commit();
+ pending = 0;
+ }
+ }
+ }
+ if (pending != 0) {
+ session.commit();
+ }
+ } catch (RuntimeException | Error failure) {
+ try {
+ session.rollback();
Review Comment:
Important: this rollback does not drop the deletes queued for the failed
batch, so they can be applied later.
Evidence, all at this head:
- `HstoreSession.delete()` calls `prepare()` (`graph.beginTx()`) and then
`graph.delete()`. In tx mode `NodeTxExecutor.prepareTx()` only appends the
operation to `entries`; nothing is sent to Store yet.
- `HstoreSession.rollback()` calls `graph.rollback()` ->
`NodeTxExecutor.rollbackTx()`, which rolls back the node sessions, sets `isTx =
false` and clears `sessions`. It never resets `entries`. Only `doCommit()`
resets it, in its `finally`.
- The session is thread-local (`BackendSessionPool.getOrNewSession()`), so
the queue stays on the task worker thread. The next write on that thread calls
`beginTx()` (just `setTx(true)`), appends, and `doCommit()` replays every
entry, including the stale deletes.
So when the scan fails or a row is rejected after 1 to 1023 matching rows
were queued (a Store scan error, or the `E.checkState` rejections above), the
task reports failure and `rollback()` returns normally, yet those deletes are
still pending. If the property is written again before that worker thread
commits something else, the replay removes the new values.
`testOlapCleanupRejects*` and `testOlapCleanupRollbackAndCloseOnCommitFailure`
cannot see this because `Session` is a mock whose `rollback()` clears the
fixture's own list.
The gap is in `NodeTxExecutor.rollbackTx()` and predates this PR, but this
path is new and the doc text added here promises that the pending session is
rolled back. Requested change: reset `entries` in `NodeTxExecutor.rollbackTx()`
(same as `doCommit()` does) and add a test that runs queue -> rollback -> next
commit against a real `NodeTxExecutor`, or validate the batch before queuing
any delete so a rejected row never leaves queued work behind.
--
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]
