imbajin commented on code in PR #2888:
URL:
https://github.com/apache/incubator-hugegraph/pull/2888#discussion_r2554954428
##########
hugegraph-server/hugegraph-mysql/src/main/java/org/apache/hugegraph/backend/store/mysql/MysqlStore.java:
##########
@@ -337,6 +337,9 @@ protected void clearTables() {
protected void truncateTables() {
Session session = this.sessions.session();
for (MysqlTable table : this.tables()) {
+ if (table instanceof MysqlTables.Meta || table instanceof
MysqlTables.Counters){
+ continue;
Review Comment:
‼️ **Critical: Fragile implementation with instanceof checks**
The current fix uses `instanceof` checks to skip specific table types during
truncation. This approach has several issues:
1. **Tight coupling**: Creates dependency on concrete implementation classes
rather than using a more abstract marker (e.g., an interface like `MetaTable`
or a table property)
2. **Not extensible**: If new meta tables are added in the future,
developers must remember to update this condition
3. **Violates Open/Closed Principle**: The store layer shouldn't need to
know about specific table implementations
```suggestion
// Consider adding a method to MysqlTable interface like:
// public boolean isSystemTable() { return false; }
// Then override in Meta and Counters tables
if (table.isSystemTable()) {
continue;
}
```
Alternatively, you could maintain a set of system tables or use table
metadata to mark which tables should be preserved.
##########
hugegraph-server/hugegraph-mysql/src/main/java/org/apache/hugegraph/backend/store/mysql/MysqlStore.java:
##########
@@ -337,6 +337,9 @@ protected void clearTables() {
protected void truncateTables() {
Session session = this.sessions.session();
for (MysqlTable table : this.tables()) {
+ if (table instanceof MysqlTables.Meta || table instanceof
MysqlTables.Counters){
Review Comment:
‼️ **Critical: Unclear why Counters table is preserved**
The PR description mentions fixing Meta table truncation but doesn't explain
why the Counters table should also be skipped. This needs clarification:
1. Is this fixing an existing bug with Counters table? (not mentioned in
issue #2209)
2. What data does the Counters table hold that should survive truncation?
3. Should Counters be reset during truncate but preserved during clear?
Please add a comment explaining the reasoning, or if this is a separate fix,
consider splitting it into a separate PR for clarity.
##########
hugegraph-server/hugegraph-mysql/src/main/java/org/apache/hugegraph/backend/store/mysql/MysqlStore.java:
##########
@@ -223,6 +222,7 @@ public void truncate() {
this.checkOpened();
this.truncateTables();
+ this.init();
Review Comment:
⚠️ **Important: Verify init() is necessary after truncate**
Adding `this.init()` after truncation appears to reinitialize the meta
table. However:
1. Does `init()` have any side effects beyond restoring meta data?
2. Is this call idempotent and safe to call multiple times?
3. Should this also be added to `clearTables()` method for consistency?
Please verify this is the correct approach and consider adding a comment
explaining why re-initialization is needed after truncate.
--
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]
