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

Let snapshot saved disk same as rocksdb data path #1392

Merged
merged 13 commits into from
Apr 1, 2021
Merged

Conversation

Linary
Copy link
Contributor

@Linary Linary commented Mar 15, 2021

  • The goal is to ensure that snapshots can be generated in hard link

Change-Id: Idfa9387e46ac094a9c4cd69fee310b271127d889

String snapshotPrefix = "snapshot";
for (BackendStore store : this.stores.values()) {
store.writeSnapshot(snapshotPrefix);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer call store.writeSnapshot() through tx, and remove writeSnapshot() from AbstractBackendStoreProvider 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.

Only memory backend will share store provider

@Linary Linary force-pushed the hardlink-snapshot branch 2 times, most recently from 9eb4eb4 to 1c8460b Compare March 22, 2021 11:20
Linary added 4 commits March 23, 2021 15:03
* The goal is to ensure that snapshots can be generated in hard link

Change-Id: Idfa9387e46ac094a9c4cd69fee310b271127d889
Change-Id: I829ad9eaaf399b5fc1b8d25a90b2d1dd88c54894
Change-Id: I2b212decb8f0639354e032a09e9ca40988b001da
Change-Id: I67d88d798c6a16a66681296fbf29d73fb8bc356a
if (ids.size() == 1) {
eventHub.call(Events.CACHE, action, type, ids.get(0));
} else {
eventHub.call(Events.CACHE, action, type, ids.toArray());
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 delete if-branch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't delete.

In CachedSchemaTransaction

if (Cache.ACTION_INVALID.equals(args[0])) {
                event.checkArgs(String.class, HugeType.class, Id.class);

the 3rd param must be Id class.

In CachedGraphTransaction

if (Cache.ACTION_INVALID.equals(args[0])) {
                event.checkArgs(String.class, HugeType.class, Object.class);

the 3rd param can be Id or Id[] class

@@ -48,7 +47,7 @@

private static final Logger LOG = Log.logger(StoreSnapshotFile.class);

private static final String SNAPSHOT_DIR = "snapshot";
public static final String SNAPSHOT_DIR = "snapshot";
Copy link
Contributor

Choose a reason for hiding this comment

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

protected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AbstractBackendStoreProvider has used it.

Change-Id: I845cc78b20b94eee6bd2aff43038dd22c1a7834a
@Linary Linary force-pushed the hardlink-snapshot branch from 1c8460b to 372657d Compare March 24, 2021 08:16
Change-Id: Iebf742426aa6b696b2397ec90368a13d04c04b56
@Linary Linary force-pushed the hardlink-snapshot branch from 372657d to 74d2a33 Compare March 24, 2021 09:00
@codecov
Copy link

codecov bot commented Mar 24, 2021

Codecov Report

Merging #1392 (319ae66) into master (126d401) will increase coverage by 0.02%.
The diff coverage is 36.87%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1392      +/-   ##
============================================
+ Coverage     62.04%   62.07%   +0.02%     
- Complexity     5838     5853      +15     
============================================
  Files           386      386              
  Lines         32192    32290      +98     
  Branches       4497     4506       +9     
============================================
+ Hits          19975    20043      +68     
- Misses        10186    10207      +21     
- Partials       2031     2040       +9     
Impacted Files Coverage Δ Complexity Δ
...ava/com/baidu/hugegraph/api/profile/GraphsAPI.java 20.75% <0.00%> (-3.69%) 0.00 <0.00> (ø)
...a/com/baidu/hugegraph/auth/HugeGraphAuthProxy.java 3.91% <0.00%> (-0.05%) 0.00 <0.00> (ø)
...e/src/main/java/com/baidu/hugegraph/HugeGraph.java 65.30% <ø> (ø) 9.00 <0.00> (ø)
...baidu/hugegraph/backend/store/BackendFeatures.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...h/backend/store/raft/RaftBackendStoreProvider.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...ugegraph/backend/store/raft/RaftSharedContext.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...ugegraph/backend/store/raft/StoreSnapshotFile.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...ugegraph/backend/store/raft/StoreStateMachine.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...gegraph/backend/store/rocksdb/RocksDBFeatures.java 78.26% <0.00%> (-3.56%) 18.00 <0.00> (ø)
...gegraph/backend/store/rocksdb/RocksDBSessions.java 100.00% <ø> (ø) 1.00 <0.00> (ø)
... and 17 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 126d401...319ae66. Read the comment docs.

RocksDB rocksdb = sessions.createSnapshotRocksDB(
snapshotPath.toString());
Path snapshotLinkPath = Paths.get(originDataPath + "_link");
createCheckpoint(rocksdb, snapshotLinkPath.toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

why createCheckpoint in resumeSnapshot()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when under raft mode, need reserve the origin snapshot dir, so need create hard link from snapshot

Change-Id: Idd420fd98be6668cf0db03178530eda3b9529ea5
snapshotLinkDir, originDataDir);
Path parentPath = snapshotPath.getParent();
if (Files.list(parentPath).count() == 0) {
FileUtils.deleteDirectory(parentPath.toFile());
Copy link
Contributor

@javeme javeme Mar 25, 2021

Choose a reason for hiding this comment

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

seems still not use "move directory" if needed to delete snapshot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't understand.

Change-Id: I29adb7df352bbbba3fbd416831b11b0968818cb0
@Linary Linary force-pushed the hardlink-snapshot branch from 699d03b to 607076f Compare March 26, 2021 08:42
Change-Id: I0561be1a71322644465e745ded9ca1636bff078a
@Linary Linary force-pushed the hardlink-snapshot branch from 607076f to fcb5860 Compare March 26, 2021 09:35
@@ -614,15 +614,12 @@ protected Session session(HugeType tableType) {
Set<String> uniqueParents = new HashSet<>();
// Every rocksdb instance should create an snapshot
for (Map.Entry<String, RocksDBSessions> entry : this.dbs.entrySet()) {
// Like: parent_path/rocksdb-data/m
// parent_path/rocksdb-vertex/g
// Like: parent_path/rocksdb-data/*, * maybe g,m,s
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe => can be

Change-Id: I045690d244ad83e81e7ea7624ae9388f88521142
@@ -322,7 +330,8 @@ public void checkPropertiesAccess() {
public void checkPropertyAccess(String key) {
if (!callFromAcceptClassLoaders() && callFromGremlin() &&
!WHITE_SYSTEM_PROPERTYS.contains(key) && !callFromBackendHbase() &&
!callFromRaft() && !callFromSofaRpc()) {
!callFromBackendRocksDB() && !callFromRaft() &&
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe exist some dup part, but indeed need it.

Change-Id: Ie7367816e681438dd4bc82287d1b810d781845aa
javeme
javeme previously approved these changes Mar 26, 2021
Change-Id: I70d1b3f70ac347f8e75a95d50654cf3f7f5536e8
javeme
javeme previously approved these changes Mar 30, 2021
Change-Id: I88b39003107652f6ff8eae071e88dc888dd21959
@Linary Linary merged commit 39b9474 into master Apr 1, 2021
@Linary Linary deleted the hardlink-snapshot branch April 1, 2021 10:05
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.

4 participants