Skip to content
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

Add system schema store #1891

Merged
merged 7 commits into from
Jun 2, 2022
Merged

Add system schema store #1891

merged 7 commits into from
Jun 2, 2022

Conversation

zyxxoo
Copy link
Contributor

@zyxxoo zyxxoo commented May 20, 2022

fix #1533

Add system schema store
Add meta table into system store
Add test case for read version
Let AbstractBackendStore impl default storedVersion()

@zyxxoo zyxxoo requested review from javeme and zhoney and removed request for javeme May 20, 2022 07:12
@zyxxoo zyxxoo force-pushed the zy_system_store branch 2 times, most recently from ace9831 to de95390 Compare May 20, 2022 07:24
@codecov
Copy link

codecov bot commented May 20, 2022

Codecov Report

Merging #1891 (77e2d49) into master (1118418) will increase coverage by 0.05%.
The diff coverage is 84.09%.

@@             Coverage Diff              @@
##             master    #1891      +/-   ##
============================================
+ Coverage     71.02%   71.08%   +0.05%     
+ Complexity      982      963      -19     
============================================
  Files           451      452       +1     
  Lines         38206    38405     +199     
  Branches       5407     5418      +11     
============================================
+ Hits          27136    27299     +163     
- Misses         8420     8447      +27     
- Partials       2650     2659       +9     
Impacted Files Coverage Δ
...e/src/main/java/com/baidu/hugegraph/HugeGraph.java 65.30% <ø> (ø)
...du/hugegraph/backend/cache/CachedBackendStore.java 15.15% <0.00%> (-0.48%) ⬇️
...om/baidu/hugegraph/backend/store/BackendStore.java 79.54% <ø> (ø)
...m/baidu/hugegraph/backend/tx/GraphTransaction.java 80.14% <ø> (ø)
...n/java/com/baidu/hugegraph/config/CoreOptions.java 99.46% <ø> (-0.02%) ⬇️
...n/java/com/baidu/hugegraph/schema/VertexLabel.java 86.95% <ø> (ø)
...c/main/java/com/baidu/hugegraph/cmd/InitStore.java 0.00% <0.00%> (ø)
...a/com/baidu/hugegraph/auth/HugeGraphAuthProxy.java 57.20% <10.00%> (-1.18%) ⬇️
.../hugegraph/backend/store/AbstractBackendStore.java 80.00% <66.66%> (-3.34%) ⬇️
...aidu/hugegraph/backend/store/raft/RaftContext.java 80.95% <66.66%> (-0.30%) ⬇️
... and 49 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1118418...77e2d49. Read the comment docs.

Add meta table into system store
Add test case for read version
Let AbstractBackendStore impl default storedVersion()
fix small bug og HugeProject
add test for SystemSchemaStore

Change-Id: I5c9d65c4ae022cb84592e8f2aa57bd7485cd12e4
@@ -70,12 +72,16 @@
public class SchemaTransaction extends IndexableTransaction {

private final SchemaIndexTransaction indexTx;
private final SystemSchemaStore systemSchemaStore;
private final LocalCounter counter;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: move LocalCounter counter define into SystemSchemaStore class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LocalCounter provider by HugegraphParams, if we move it to SystemSchemaStore, maybe change too much code

@@ -25,9 +25,11 @@
public abstract class AbstractBackendStore<Session extends BackendSession>
implements BackendStore {

private final SystemSchemaStore systemSchemaStore;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: move SystemSchemaStore into backend like MetaStore

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this TODO annotation it first

public BackendStoreInfo backendStoreInfo() {
// Just for trigger Tx.getOrNewTransaction, then load 3 stores
this.systemTransaction();
return new BackendStoreInfo(this.configuration, this.storeProvider);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: pass storeProvider.metaStore()

@zyxxoo zyxxoo force-pushed the zy_system_store branch from de95390 to 3e332b4 Compare May 30, 2022 12:53
@javeme
Copy link
Contributor

javeme commented May 30, 2022

mysql ci:

Error:  Tests run: 728, Failures: 0, Errors: 2, Skipped: 36, Time elapsed: 372.603 s <<< FAILURE! - in com.baidu.hugegraph.core.CoreTestSuite
Error:  testCreateGraphsWithSameName(com.baidu.hugegraph.core.MultiGraphsTest)  Time elapsed: 0.436 s  <<< ERROR!
com.baidu.hugegraph.backend.BackendException: Failed to insert driver version with 'INSERT INTO m VALUES ('VERSION', '1.11')'
	at com.baidu.hugegraph.core.MultiGraphsTest.testCreateGraphsWithSameName(MultiGraphsTest.java:263)

Error:  testWriteAndReadVersion(com.baidu.hugegraph.core.MultiGraphsTest)  Time elapsed: 0.206 s  <<< ERROR!
com.baidu.hugegraph.backend.BackendException: Failed to insert driver version with 'INSERT INTO m VALUES ('VERSION', '1.11')'
	at com.baidu.hugegraph.core.MultiGraphsTest.testWriteAndReadVersion(MultiGraphsTest.java:65)

driverVersion);
try {
session.execute(insert);
} catch (SQLException throwables) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to SQLException e and pass into BackendException

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

method sign is session.execute() throw SQLException, so we can't change to BackendException

@zyxxoo zyxxoo force-pushed the zy_system_store branch from bc5b071 to c8781e8 Compare May 30, 2022 15:38
versionColumn);
try {
session.execute(insert);
} catch (SQLException throwables) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@zyxxoo zyxxoo force-pushed the zy_system_store branch from c8781e8 to 79603ae Compare May 30, 2022 15:44
@javeme
Copy link
Contributor

javeme commented May 31, 2022

ci error during init-store:

Error: Exception in thread "main" java.lang.IllegalArgumentException: Undefined vertex label: '~user'
	at com.google.common.base.Preconditions.checkArgument(Preconditions.java:163)
	at com.baidu.hugegraph.util.E.checkArgument(E.java:56)
	at com.baidu.hugegraph.StandardHugeGraph.vertexLabel(StandardHugeGraph.java:793)
	at com.baidu.hugegraph.auth.HugeGraphAuthProxy.lambda$vertexLabel$2(HugeGraphAuthProxy.java:259)
	at com.baidu.hugegraph.auth.HugeGraphAuthProxy.lambda$verifySchemaPermission$24(HugeGraphAuthProxy.java:914)
	at com.baidu.hugegraph.auth.HugeGraphAuthProxy.verifyResPermission(HugeGraphAuthProxy.java:939)
	at com.baidu.hugegraph.auth.HugeGraphAuthProxy.verifyResPermission(HugeGraphAuthProxy.java:925)
	at com.baidu.hugegraph.auth.HugeGraphAuthProxy.verifySchemaPermission(HugeGraphAuthProxy.java:912)
	at com.baidu.hugegraph.auth.HugeGraphAuthProxy.verifySchemaPermission(HugeGraphAuthProxy.java:905)
	at com.baidu.hugegraph.auth.HugeGraphAuthProxy.vertexLabel(HugeGraphAuthProxy.java:258)
	at com.baidu.hugegraph.auth.EntityManager.queryEntity(EntityManager.java:154)
	at com.baidu.hugegraph.auth.EntityManager.query(EntityManager.java:136)
	at com.baidu.hugegraph.auth.StandardAuthManager.findUser(StandardAuthManager.java:177)
	at com.baidu.hugegraph.auth.StandardAuthenticator.requireInitAdminUser(StandardAuthenticator.java:77)
	at com.baidu.hugegraph.auth.StandardAuthenticator.initAdminUser(StandardAuthenticator.java:52)
	at com.baidu.hugegraph.auth.StandardAuthenticator.initAdminUserIfNeeded(StandardAuthenticator.java:191)
	at com.baidu.hugegraph.cmd.InitStore.main(InitStore.java:91)

import org.junit.Before;
import org.junit.Test;

import com.baidu.hugegraph.testutil.Assert;
import com.google.common.collect.ImmutableMap;

import jakarta.ws.rs.core.Response;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems can remove

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it had been used, so we can't remove

@@ -998,6 +998,12 @@ public HugeGraph graph() {
return this.taskScheduler.graph();
}

@Override
public void init() {
verifyAdminPermission();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.verifyAdminPermission()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this method doesn't use this, the code is in inner class TaskSchedulerProxy

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get it

@@ -1161,6 +1167,12 @@ private String currentUsername() {
return null;
}

@Override
public void init() {
verifyAdminPermission();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is inner class AuthManagerProxy, so can't use this.

@zyxxoo zyxxoo force-pushed the zy_system_store branch from 2445d75 to 1c9f0b4 Compare June 1, 2022 16:09
String driverVersion = this.provider().driverVersion();
this.meta.writeVersion(session, driverVersion);
this.meta.writeVersion(this.session(), driverVersion);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove "this.checkOpened()" since called in this.session()

@@ -578,7 +574,7 @@ protected List<String> tableNames() {
public void init() {
super.init();
super.checkOpened();
Session session = super.getSession();
Session session = super.sessions.session();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can call this.session(null), and we can also replace the following code with
Session session = this.session(null) like line 228,477:

this.checkOpened();
Session session = this.sessions.session();

@@ -1118,8 +1114,7 @@ public synchronized void init() {
Lock writeLock = this.storeLock().writeLock();
writeLock.lock();
try {
super.checkOpened();
Session session = super.getSession();
Session session = super.sessions.session();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can call this.session(HugeType.META)

@@ -800,14 +795,13 @@ public void init() {
super.init();
this.checkOpened();
String driverVersion = this.provider().driverVersion();
this.meta.writeVersion(this.session(), driverVersion);
this.meta.writeVersion(this.session(null), driverVersion);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this.checkOpened() call.
also update other unused call "checkOpened()"?

@@ -1129,7 +1129,7 @@ public String storedVersion() {
readLock.lock();
try {
super.checkOpened();
Session session = super.sessions.session();
Session session = super.session(null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this.checkOpened() call

@@ -1114,7 +1114,7 @@ public synchronized void init() {
Lock writeLock = this.storeLock().writeLock();
writeLock.lock();
try {
Session session = super.sessions.session();
Session session = super.session(HugeType.META);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.session(HugeType.META);

@@ -1129,7 +1129,7 @@ public String storedVersion() {
readLock.lock();
try {
super.checkOpened();
Session session = super.sessions.session();
Session session = super.session(null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep this.session(HugeType.META)

@@ -573,17 +573,15 @@ protected List<String> tableNames() {
@Override
public void init() {
super.init();
super.checkOpened();
Session session = super.sessions.session();
Session session = super.session(null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep this.session(null) style?

also update other "super.session("?

@@ -998,6 +998,12 @@ public HugeGraph graph() {
return this.taskScheduler.graph();
}

@Override
public void init() {
verifyAdminPermission();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get it

@Override
public void init() {
super.init();
this.checkOpened();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@javeme javeme merged commit 2af01bc into master Jun 2, 2022
@javeme javeme deleted the zy_system_store branch June 2, 2022 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants