refact(rocksdb): clean & reformat some code by imbajin · Pull Request #2200 · apache/hugegraph

imbajin · 2023-04-17T11:24:09Z

also mark TODO in some gists

split it for future dev

TODO:

fix the CodeQL build error problem (try to use our cmd to build)

also mark TODO in some gists

javeme · 2023-04-17T12:45:55Z

hugegraph-rocksdb/src/main/java/org/apache/hugegraph/backend/store/rocksdb/RocksDBTables.java

        }

        @Override
+        // TODO: why this method is same as super.eliminate() in RocksDBTable, del it?


added the assert?

added the assert?

seems totally same in parent class?

https://github.com/apache/incubator-hugegraph/blob/e7c7558acdc8a27167dc5f2f36168a3ef4423b53/hugegraph-rocksdb/src/main/java/org/apache/hugegraph/backend/store/rocksdb/RocksDBTable.java#L114

I checked and find out that delete() method is override by IndexTable, we want to call super.delete() instead of this.delete() here. we can add a NOTE here.

javeme · 2023-04-17T12:47:43Z

hugegraph-rocksdb/src/main/java/org/apache/hugegraph/backend/store/rocksdb/RocksDBTable.java


        @Override
        public byte[] position(String position) {
+            // TODO: START & END is same & be empty now? remove one?


yes, but the meanings are different

javeme · 2023-04-17T12:48:45Z

...aph-rocksdb/src/main/java/org/apache/hugegraph/backend/store/rocksdb/RocksDBStdSessions.java

        E.checkArgument(snapshotPath.toFile().exists(),
-                        "The snapshot path '%s' doesn't exist",
-                        snapshotPath);
+                        "The snapshot path '%s' doesn't exist", snapshotPath);


it's more clear to keep 3 lines

it's more clear to keep 3 lines

seems one phrase for one line to read is better? like

// ① not split throw new BackendException("Table '%s' is not opened", cfName); // ② split, not necessary to break one line unless it's too long? throw new BackendException("Table '%s' is not opened", cfName);

javeme · 2023-04-17T12:49:49Z

...aph-rocksdb/src/main/java/org/apache/hugegraph/backend/store/rocksdb/RocksDBStdSessions.java

-    private static Set<String> mergeOldCFs(String path, List<String> cfNames)
-                                           throws RocksDBException {
+    private static Set<String> mergeOldCFs(String path,
+                                           List<String> cfNames) throws RocksDBException {


prefer the old style

currently, the Exception keyword can't be auto formatted well, so prefer not to align it in a single line

otherwise, it will break the style while formatting the code (with plugin)

javeme · 2023-04-19T12:53:36Z

compile error: https://github.com/apache/incubator-hugegraph/pull/2199/files#diff-3744a042d40e54198e87c7271da06363141165f2523541c71e81c820486ca87fR38

Update:
fetched from master code (Found a problem, if we disable "require latest code", we also loose the button to update from the master in GitHub's UI.. 😿 )

imbajin · 2023-04-20T07:56:48Z

@zyxxoo more context about the DriverVersion & StoreVersion? CI alert error log

And why alert this (without any code logic change)

fix

imbajin · 2023-04-20T08:02:49Z

...ph-rocksdb/src/main/java/org/apache/hugegraph/backend/store/rocksdb/RocksDBIteratorPool.java

+        // TODO: is the typo "EREUSING_ENABLED" right? or should be "REUSING_ENABLED"?
        private static final boolean EREUSING_ENABLED = false;


need ensure

javeme · 2023-09-10T12:41:54Z

...aph-rocksdb/src/main/java/org/apache/hugegraph/backend/store/rocksdb/RocksDBStdSessions.java

-    private static Set<String> mergeOldCFs(String path, List<String> cfNames)
-                                           throws RocksDBException {
+    private static Set<String> mergeOldCFs(String path,
+                                           List<String> cfNames) throws RocksDBException {


javeme · 2023-09-10T12:44:19Z

hugegraph-rocksdb/src/main/java/org/apache/hugegraph/backend/store/rocksdb/RocksDBStore.java


            for (Map.Entry<String, RocksDBSessions> entry :
-                 snapshotPaths.entrySet()) {
+                snapshotPaths.entrySet()) {


keep in one line?

javeme · 2023-09-10T12:49:43Z

hugegraph-rocksdb/src/main/java/org/apache/hugegraph/backend/store/rocksdb/RocksDBTable.java


        @Override
        public byte[] position(String position) {
+            // TODO: START & END is same & be empty now? remove one?


the values are equaled, but the meanings are different, we can add NOTE for the const define

javeme · 2023-09-10T12:57:13Z

hugegraph-rocksdb/src/main/java/org/apache/hugegraph/backend/store/rocksdb/RocksDBTables.java

        }

        @Override
+        // TODO: why this method is same as super.eliminate() in RocksDBTable, del it?


I checked and find out that delete() method is override by IndexTable, we want to call super.delete() instead of this.delete() here. we can add a NOTE here.

… into clean-rocksdb

VGalaxies · 2023-12-11T04:39:24Z

...-rocksdb/src/main/java/org/apache/hugegraph/backend/store/rocksdbsst/RocksDBSstSessions.java

-                        for (Pair<byte[], byte[]> change : table.getValue()) {
-                            sst.put(change.getKey(), change.getValue());
-                        }
+                    SstFileWriter sst = table(table.getKey());


FIX ME: 'SstFileWriter' used without 'try'-with-resources statement

* chore: merge master to clean-rocksdb for synchronization (apache#2383) --------- Co-authored-by: V_Galaxy <[email protected]>

refact(rocksdb): clean & reformat some code

e7c7558

also mark TODO in some gists

javeme reviewed Apr 17, 2023

View reviewed changes

clean more code

eab530c

Merge branch 'master' into clean-rocksdb

4bdfe34

This comment was marked as off-topic.

Sign in to view

RocksDB tests

851ac3b

fix

imbajin commented Apr 20, 2023

View reviewed changes

github-actions bot added the inactive label May 20, 2023

javeme added rocksdb RocksDB backend and removed inactive labels May 31, 2023

github-actions bot added the inactive label Jun 30, 2023

javeme removed the inactive label Jul 1, 2023

github-actions bot added the inactive label Jul 31, 2023

This comment was marked as outdated.

Sign in to view

imbajin added 2 commits September 10, 2023 17:07

Merge branch 'master' into clean-rocksdb

d467b02

tiny improve

1b8565a

javeme reviewed Sep 10, 2023

View reviewed changes

github-actions bot removed the inactive label Sep 10, 2023

github-actions bot added the inactive label Oct 10, 2023

javeme removed the inactive label Oct 11, 2023

imbajin added this to the 1.2.0 milestone Oct 27, 2023

imbajin mentioned this pull request Dec 4, 2023

[Summary] Server Release v1.2.0 TODOs #2375

Open

3 tasks

Empty test

70f938c

apache deleted a comment from github-actions bot Dec 11, 2023

VGalaxies mentioned this pull request Dec 11, 2023

chore: merge master to clean-rocksdb for synchronization #2383

Merged

11 tasks

chore: merge master to clean-rocksdb for synchronization (#2383)

7cec2a7

imbajin added 2 commits December 11, 2023 12:30

Merge branch 'master' into clean-rocksdb

a9cb966

Merge branch 'clean-rocksdb' of github.com:apache/incubator-hugegraph…

2c7baf0

… into clean-rocksdb

VGalaxies reviewed Dec 11, 2023

View reviewed changes

apache deleted a comment from github-actions bot Dec 11, 2023

simon824 approved these changes Dec 11, 2023

View reviewed changes

z7658329 approved these changes Dec 11, 2023

View reviewed changes

imbajin merged commit bd5d68f into master Dec 11, 2023

imbajin deleted the clean-rocksdb branch December 11, 2023 14:21

		// TODO: is the typo "EREUSING_ENABLED" right? or should be "REUSING_ENABLED"?
		private static final boolean EREUSING_ENABLED = false;

Conversation

imbajin commented Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imbajin Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javeme commented Apr 19, 2023 • edited by imbajin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as off-topic.

imbajin commented Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

imbajin commented Apr 17, 2023 •

edited

Loading

imbajin Apr 17, 2023 •

edited

Loading

javeme commented Apr 19, 2023 •

edited by imbajin

Loading

imbajin commented Apr 20, 2023 •

edited

Loading