-
Notifications
You must be signed in to change notification settings - Fork 571
fix(server): ensure graph clear API does not clear meta table for hbase #2911
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 3 commits
fbb5e04
9c810fd
004684e
7676f54
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -114,6 +114,13 @@ protected List<String> tableNames() { | |||||||||||||||||
| .collect(Collectors.toList()); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| protected List<String> truncatedTableNames() { | ||||||||||||||||||
| return this.tables.entrySet().stream() | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While unlikely, if
Suggested change
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current code 'HugeType.META == e.getKey()' is already NPE-safe regardless of which operand is null, as the '==' operator never throws NullPointerException. |
||||||||||||||||||
| .filter(e -> !(HugeType.META == e.getKey())) | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The filter condition Consider adding a comment explaining why META type should be preserved during truncation to improve code maintainability.
Suggested change
|
||||||||||||||||||
| .map(e -> e.getValue().table()) | ||||||||||||||||||
|
Comment on lines
+119
to
+121
|
||||||||||||||||||
| return this.tables.entrySet().stream() | |
| .filter(e -> !(HugeType.META == e.getKey())) | |
| .map(e -> e.getValue().table()) | |
| return this.tables.values().stream() | |
| .map(BackendTable::table) |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,63 @@ | ||||||||||||||||||||||||||||||||||||||||||||
| /* | ||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||
| * * 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.unit.hbase; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| import org.apache.commons.configuration2.Configuration; | ||||||||||||||||||||||||||||||||||||||||||||
| import org.apache.hugegraph.backend.store.hbase.HbaseSessions; | ||||||||||||||||||||||||||||||||||||||||||||
| import org.apache.hugegraph.backend.store.hbase.HbaseStoreProvider; | ||||||||||||||||||||||||||||||||||||||||||||
| import org.apache.hugegraph.config.HugeConfig; | ||||||||||||||||||||||||||||||||||||||||||||
| import org.apache.hugegraph.testutil.Utils; | ||||||||||||||||||||||||||||||||||||||||||||
| import org.apache.hugegraph.unit.BaseUnitTest; | ||||||||||||||||||||||||||||||||||||||||||||
| import org.junit.After; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| import java.io.IOException; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Naming: Space missing in class declaration Minor style issue:
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
| public class BaseHbaseUnitTest extends BaseUnitTest{ | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
| public class BaseHbaseUnitTest extends BaseUnitTest{ | |
| public class BaseHbaseUnitTest extends BaseUnitTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Before annotation
The method is named setup() but the annotation @Before is missing. JUnit won't automatically call this method before each test. This could cause tests to fail or behave unexpectedly.
| protected HbaseSessions sessions; | |
| @Before | |
| public void setup() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If any exception occurs after opening stores but before sessions.open(), the opened stores won't be properly closed. This can lead to resource leaks in test execution.
| this.provider = new HbaseStoreProvider(); | |
| @Before | |
| public void setup() throws IOException { | |
| Configuration conf = Utils.getConf(); | |
| this.config = new HugeConfig(conf); | |
| this.provider = new HbaseStoreProvider(); | |
| try { | |
| this.provider.open(GRAPH_NAME); | |
| this.provider.loadSystemStore(config).open(config); | |
| this.provider.loadGraphStore(config).open(config); | |
| this.provider.loadSchemaStore(config).open(config); | |
| this.provider.init(); | |
| this.sessions = new HbaseSessions(config, GRAPH_NAME, | |
| this.provider.loadGraphStore(config).store()); | |
| this.sessions.open(); | |
| } catch (Exception e) { | |
| tearDown(); | |
| throw e; | |
| } | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For code consistency, add a space after commas in method calls.
| this.provider.loadSystemStore(config).open(config); | |
| this.sessions = new HbaseSessions(config, GRAPH_NAME, this.provider.loadGraphStore(config).store()); |
imbajin marked this conversation as resolved.
Show resolved
Hide resolved
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
down() should be tearDown()
The cleanup method should follow JUnit naming conventions and be named tearDown() to match the common pattern.
| this.sessions = new HbaseSessions(config,GRAPH_NAME, this.provider.loadGraphStore(config).store()); | |
| @After | |
| public void tearDown() { |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| this.sessions = new HbaseSessions(config,GRAPH_NAME, this.provider.loadGraphStore(config).store()); | |
| public void tearDown() { |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sessions field is opened in setup() but never closed in the cleanup method. This could lead to HBase connection leaks.
| this.sessions = new HbaseSessions(config,GRAPH_NAME, this.provider.loadGraphStore(config).store()); | |
| @After | |
| public void tearDown() { | |
| if (this.sessions != null) { | |
| try { | |
| this.sessions.close(); | |
| } catch (Exception e) { | |
| LOG.warn("Failed to close sessions", e); | |
| } | |
| } | |
| if (this.provider != null) { |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Code quality: Silent exception swallowing
Using empty catch blocks makes debugging difficult. Consider:
| public void down(){ | |
| @After | |
| public void down(){ | |
| if (this.store != null) { | |
| try { | |
| this.store.close(); | |
| } catch (Exception e) { | |
| LOG.warn("Failed to close store", e); | |
| } | |
| } | |
| if (this.provider != null) { | |
| try { | |
| this.provider.close(); | |
| } catch (Exception e) { | |
| LOG.warn("Failed to close provider", e); | |
| } | |
| } | |
| } |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public void down(){ | |
| LOG.warn("Failed to close provider", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Code style: inconsistent spacing
Missing space after catch keyword.
| this.provider.close(); | |
| } catch (Exception e) { |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,89 @@ | ||||||||||||||||||||||||||||
| /* | ||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||
| * * 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.unit.hbase; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| import org.apache.hadoop.hbase.client.Result; | ||||||||||||||||||||||||||||
| import org.apache.hugegraph.backend.store.BackendEntry.BackendIterator; | ||||||||||||||||||||||||||||
| import org.apache.hugegraph.backend.store.BackendStore; | ||||||||||||||||||||||||||||
| import org.apache.hugegraph.testutil.Assert; | ||||||||||||||||||||||||||||
| import org.apache.hugegraph.backend.store.hbase.HbaseSessions; | ||||||||||||||||||||||||||||
| import org.apache.hugegraph.util.StringEncoding; | ||||||||||||||||||||||||||||
| import org.junit.Before; | ||||||||||||||||||||||||||||
| import org.junit.Test; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| import java.io.IOException; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| public class HbaseUnitTest extends BaseHbaseUnitTest { | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| @Before | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
| @Before | |
| @Before | |
| public void setUp(){ | |
| super.setup(); // Call parent setup first | |
| this.provider.open(GRAPH_NAME); | |
| this.provider.init(); | |
| } | |
| @After | |
| public void teardown(){ | |
| // Remove duplicate cleanup - handled by parent | |
| // Only add child-specific cleanup here if needed | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test only verifies version equality but doesn't validate:
- Meta table content: Check that specific meta entries (like backend version, graph name, etc.) are preserved
- Data table clearing: Verify that actual graph data (vertices, edges) are properly cleared
- Concurrent operations: Test behavior when truncate is called during other operations
Suggestion:
@Test
public void testHbaseMetaVersion(){
// Insert some test data
// ... add vertices/edges ...
String beforeVersion = this.store.storedVersion();
// Get other meta entries
this.store.truncate();
String afterVersion = this.store.storedVersion();
Assert.assertEquals(beforeVersion, afterVersion);
// Verify data is cleared but meta is intact
// ... check vertices/edges are gone ...
// ... check other meta entries preserved ...
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test only verifies that meta version is preserved, but doesn't verify the actual truncation behavior. Consider adding assertions to verify:
- Data tables are actually truncated (e.g., insert some data, truncate, verify data is gone)
- Meta table content remains intact
- The graph can be used normally after truncation
Example enhancement:
@Test
public void testHbaseMetaVersionAfterTruncate() {
BackendStore systemStore = this.provider.loadSystemStore(config);
BackendStore graphStore = this.provider.loadGraphStore(config);
// Record initial version
String beforeVersion = systemStore.storedVersion();
// Insert some test data to verify truncation
// ... add test data insertion code ...
// Perform truncation
this.provider.truncate();
// Verify version preserved
String afterVersion = systemStore.storedVersion();
Assert.assertEquals(beforeVersion, afterVersion);
// Verify data tables are empty
// ... add verification code ...
}
imbajin marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test inserts data with specific row keys (row_trunc_v, row_trunc_oe, row_trunc_ie). If this test runs multiple times or fails mid-execution, residual data might affect subsequent runs.
Consider:
- Using unique row keys per test run (e.g., append timestamp/UUID)
- Adding explicit cleanup in
@Beforesetup - Verifying the
@Afterteardown properly cleans all test data
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an assertion fails, the subsequent close() calls won't execute. Consider using try-with-resources or try-finally blocks.
| testsession.put("g_oe", "f".getBytes(), "row_trunc_oe".getBytes(), StringEncoding.encode("q"), StringEncoding.encode("v")); | |
| // Verify data insertion success | |
| try (BackendIterator<Result> vIterator = testsession.get("g_v", "f".getBytes(), "row_trunc_v".getBytes()); | |
| BackendIterator<Result> oeIterator = testsession.get("g_oe", "f".getBytes(), "row_trunc_oe".getBytes()); | |
| BackendIterator<Result> ieIterator = testsession.get("g_ie", "f".getBytes(), "row_trunc_ie".getBytes())) { | |
| Assert.assertTrue("data should exist", vIterator.hasNext()); | |
| Assert.assertTrue("data should exist", oeIterator.hasNext()); | |
| Assert.assertTrue("data should exist", ieIterator.hasNext()); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| String afterVersion = systemStore.storedVersion(); | |
| Assert.assertNotNull("System metadata version should exist", afterVersion); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method name
truncatedTableNames()is grammatically incorrect and misleading. It suggests tables that have already been truncated, when it actually returns tables that SHOULD be truncated.Alternative names:
getClearableTableNames(),getDataTableNames(), orgetTableNamesExcludingMeta()