feat(pd): integrate `pd-core` into hugegraph #2478

VGalaxies · 2024-03-14T06:13:03Z

subtask of #2265

The corresponding tests will be merged after pd-service

For detailed module analysis documentation, please refer to this link.

…to hugegraph (#2460) subtask of #2265 --- During the code review, I found the following issues: 1. Similar functionality appears multiple times, such as stub connection-related code, with redundancy between `PDClient.StubProxy` and `AbstractClientStubProxy`. 2. Package partitioning: 1. `PDPulse`, `PDPulseImpl` should in `pulse` 2. `PDWatch`, `PDWatchImpl` should in `watch` 3. Unused code, see below --------- Co-authored-by: imbajin <[email protected]>

codecov · 2024-03-14T06:14:49Z

Codecov Report

Attention: Patch coverage is 0% with 3102 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (pd-store-dev@894bf98). Click here to learn what that means.

Files	Patch %	Lines
...java/org/apache/hugegraph/pd/PartitionService.java	0.00%	735 Missing ⚠️
...java/org/apache/hugegraph/pd/StoreNodeService.java	0.00%	551 Missing ⚠️
...a/org/apache/hugegraph/pd/TaskScheduleService.java	0.00%	456 Missing ⚠️
...rg/apache/hugegraph/pd/meta/MetadataKeyHelper.java	0.00%	178 Missing ⚠️
...a/org/apache/hugegraph/pd/store/HgKVStoreImpl.java	0.00%	172 Missing ⚠️
...c/main/java/org/apache/hugegraph/pd/KvService.java	0.00%	146 Missing ⚠️
...va/org/apache/hugegraph/pd/meta/PartitionMeta.java	0.00%	115 Missing ⚠️
...g/apache/hugegraph/pd/StoreMonitorDataService.java	0.00%	114 Missing ⚠️
...java/org/apache/hugegraph/pd/meta/IdMetaStore.java	0.00%	102 Missing ⚠️
.../java/org/apache/hugegraph/pd/config/PDConfig.java	0.00%	95 Missing ⚠️
... and 16 more

Additional details and impacted files

@@               Coverage Diff               @@
##             pd-store-dev    #2478   +/-   ##
===============================================
  Coverage                ?   59.33%           
  Complexity              ?      827           
===============================================
  Files                   ?      558           
  Lines                   ?    47575           
  Branches                ?     6479           
===============================================
  Hits                    ?    28228           
  Misses                  ?    16530           
  Partials                ?     2817

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/ZipUtils.java

hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/config/PDConfig.java

VGalaxies · 2024-03-17T07:29:30Z

@imbajin Do we need to address the Code scanning issues in this PR?

imbajin · 2024-03-17T07:44:34Z

@imbajin Do we need to address the Code scanning issues in this PR?

miss the context, any details?

VGalaxies · 2024-03-17T07:46:20Z

@imbajin Do we need to address the Code scanning issues in this PR?

miss the context, any details?

https://github.com/apache/incubator-hugegraph/pull/2478/checks?check_run_id=22664435037

javeme · 2024-03-23T13:15:41Z

hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/meta/MetadataRocksDBStore.java

+
+    @Override
+    public void close() {
+


missing close

javeme · 2024-03-23T13:16:37Z

hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/meta/MetadataStoreBase.java

+
+import com.google.protobuf.Parser;
+
+public abstract class MetadataStoreBase {


prefer interface MetadataStore

javeme · 2024-03-23T13:17:48Z

hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/meta/PartitionMeta.java

+ * 分区信息管理
+ */
+@Slf4j
+public class PartitionMeta extends MetadataRocksDBStore {


can we let PartitionMeta hold a MetadataRocksDBStore instead?

javeme · 2024-03-23T13:18:50Z

hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/meta/QueueStore.java

+
+public class QueueStore extends MetadataRocksDBStore {
+
+    QueueStore(PDConfig pdConfig) {


can we keep public/protected mark explicitly

javeme · 2024-03-23T13:21:38Z

hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/meta/QueueStore.java

+import org.apache.hugegraph.pd.raft.RaftEngine;
+import org.apache.hugegraph.pd.store.RaftKVStore;
+
+public class QueueStore extends MetadataRocksDBStore {


can we keep a consistent naming style: QueueMetaStore or QueueMeta

javeme · 2024-03-23T13:55:43Z

hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/LogService.java

+
+@Slf4j
+@Service
+public class LogService {


can we clarify XxLogService

javeme · 2024-03-23T13:59:35Z

hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/StoreStatusListener.java

+
+import org.apache.hugegraph.pd.grpc.Metapb;
+
+public interface StoreStatusListener {


does the Store mean StoreNode? if yes we can keep StoreNodeStatusListener.

javeme · 2024-03-23T14:01:53Z

hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/TaskScheduleService.java

+ * 4、监测Partition是否需要分裂,监测分裂是否完成
+ */
+@Slf4j
+public class TaskScheduleService {


seems it's not a service, prefer to keep TaskScheduler name, and also move to a package
like pd.task

javeme · 2024-03-23T14:04:12Z

hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/ShardGroupStatusListener.java

+
+import org.apache.hugegraph.pd.grpc.Metapb;
+
+public interface ShardGroupStatusListener {


can we move all the Listener files to package pd.listener, and move all the Service files to package pd.service

javeme · 2024-03-23T14:07:16Z

hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/TaskScheduleService.java

+        });
+        return movedPartitions;
+    }
+


useless blank line

javeme · 2024-03-23T17:17:51Z

One more suggestion, we can split the files under the pd client into multiple directories according to functions, and provide Clients.java for navigation(like Clients.createXxClient()). In addition, the files naming of pd proto need to be classified/formatted.

imbajin · 2024-03-24T13:58:14Z

One more suggestion, we can split the files under the pd client into multiple directories according to functions, and provide Clients.java for navigation(like Clients.createXxClient()). In addition, the files naming of pd proto need to be classified/formatted.

@javeme @zyxxoo

The code-style related issues(name/comment/format/clean) may list first(although we already cleaned a lot of them..), the style problems are better to be modified together after reaching TODOs to avoid more inconsistencies under version merge.
For now, we can first focus on the problems in code logic and implementation, which can only be considered as a tradeoff 😿 (so as the PR #2476 )

VGalaxies · 2024-04-02T03:10:32Z

merge directly into the master branch after #2498...

codecov-commenter · 2024-04-04T09:43:04Z

Codecov Report

Attention: Patch coverage is 0% with 3102 lines in your changes are missing coverage. Please review.

Project coverage is 57.13%. Comparing base (3a1618f) to head (c1ca5bf).
Report is 12 commits behind head on master.

Files	Patch %	Lines
...java/org/apache/hugegraph/pd/PartitionService.java	0.00%	735 Missing ⚠️
...java/org/apache/hugegraph/pd/StoreNodeService.java	0.00%	551 Missing ⚠️
...a/org/apache/hugegraph/pd/TaskScheduleService.java	0.00%	456 Missing ⚠️
...rg/apache/hugegraph/pd/meta/MetadataKeyHelper.java	0.00%	178 Missing ⚠️
...a/org/apache/hugegraph/pd/store/HgKVStoreImpl.java	0.00%	172 Missing ⚠️
...c/main/java/org/apache/hugegraph/pd/KvService.java	0.00%	146 Missing ⚠️
...va/org/apache/hugegraph/pd/meta/PartitionMeta.java	0.00%	115 Missing ⚠️
...g/apache/hugegraph/pd/StoreMonitorDataService.java	0.00%	114 Missing ⚠️
...java/org/apache/hugegraph/pd/meta/IdMetaStore.java	0.00%	102 Missing ⚠️
.../java/org/apache/hugegraph/pd/config/PDConfig.java	0.00%	95 Missing ⚠️
... and 16 more

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2478      +/-   ##
============================================
- Coverage     65.64%   57.13%   -8.52%     
  Complexity      827      827              
============================================
  Files           518      605      +87     
  Lines         42987    49334    +6347     
  Branches       5976     6639     +663     
============================================
- Hits          28218    28186      -32     
- Misses        11957    18333    +6376     
- Partials       2812     2815       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

VGalaxies · 2024-04-15T03:41:06Z

@imbajin All Code scanning warning have been fixed except for this one:

imbajin

THX ❤️~

VGalaxies and others added 6 commits March 11, 2024 17:03

Merge branch 'master' into pd-store-dev

894bf98

git add hugegraph-pd/hg-pd-core/

ffe5625

adjust hugegraph-pd/pom.xml

24ad274

format hg-pd-core

d40b270

add deps for hg-pd-test

cc156bf

VGalaxies added the pd PD module label Mar 14, 2024

VGalaxies self-assigned this Mar 14, 2024

VGalaxies mentioned this pull request Mar 14, 2024

[Summary] integrate new modules PD and Store components into hugegraph (Breaking Change) #2265

Closed

26 tasks

github-advanced-security bot found potential problems Mar 14, 2024

View reviewed changes

hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/ZipUtils.java Fixed Show fixed Hide fixed

hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/config/PDConfig.java Dismissed Show resolved Hide resolved

imbajin requested review from zyxxoo and imbajin March 14, 2024 09:54

fix comment

b7c9b68

VGalaxies mentioned this pull request Mar 16, 2024

[Summary] integrate new module PD into hugegraph #2481

Closed

9 tasks

javeme reviewed Mar 23, 2024

View reviewed changes

VGalaxies force-pushed the pd-store-dev branch from 4b2e99a to b8ed862 Compare April 3, 2024 16:01

VGalaxies changed the base branch from pd-store-dev to master April 4, 2024 03:07

Merge branch 'master' into intro-pd-core

f71141c

VGalaxies changed the title ~~feat(pd-dev): integrate pd-core into hugegraph~~ feat(pd): integrate pd-core into hugegraph Apr 4, 2024

VGalaxies marked this pull request as ready for review April 4, 2024 09:42

dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. feature New feature labels Apr 4, 2024

imbajin added this to the 1.5.0 milestone Apr 5, 2024

fix: arbitrary file access during archive extraction

c1ca5bf

imbajin approved these changes Apr 19, 2024

View reviewed changes

dosubot bot added the lgtm This PR has been approved by a maintainer label Apr 19, 2024

liuxiaocs7 approved these changes Apr 19, 2024

View reviewed changes

imbajin merged commit 43cae1d into master Apr 19, 2024
14 checks passed

imbajin deleted the intro-pd-core branch April 19, 2024 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(pd): integrate `pd-core` into hugegraph #2478

feat(pd): integrate `pd-core` into hugegraph #2478

VGalaxies commented Mar 14, 2024 •

edited

Loading

codecov bot commented Mar 14, 2024 •

edited

Loading

VGalaxies commented Mar 17, 2024 •

edited

Loading

imbajin commented Mar 17, 2024

VGalaxies commented Mar 17, 2024

javeme Mar 23, 2024

javeme Mar 23, 2024

javeme Mar 23, 2024

javeme Mar 23, 2024

javeme Mar 23, 2024

javeme Mar 23, 2024

javeme Mar 23, 2024

javeme Mar 23, 2024

javeme Mar 23, 2024

javeme Mar 23, 2024

javeme commented Mar 23, 2024

imbajin commented Mar 24, 2024 •

edited

Loading

VGalaxies commented Apr 2, 2024

codecov-commenter commented Apr 4, 2024 •

edited

Loading

VGalaxies commented Apr 15, 2024

imbajin left a comment


		import com.google.protobuf.Parser;

		public abstract class MetadataStoreBase {


		public class QueueStore extends MetadataRocksDBStore {

		QueueStore(PDConfig pdConfig) {


		import org.apache.hugegraph.pd.grpc.Metapb;

		public interface StoreStatusListener {


		import org.apache.hugegraph.pd.grpc.Metapb;

		public interface ShardGroupStatusListener {

feat(pd): integrate pd-core into hugegraph #2478

feat(pd): integrate pd-core into hugegraph #2478

Conversation

VGalaxies commented Mar 14, 2024 • edited Loading

codecov bot commented Mar 14, 2024 • edited Loading

Codecov Report

VGalaxies commented Mar 17, 2024 • edited Loading

imbajin commented Mar 17, 2024

VGalaxies commented Mar 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javeme commented Mar 23, 2024

imbajin commented Mar 24, 2024 • edited Loading

VGalaxies commented Apr 2, 2024

codecov-commenter commented Apr 4, 2024 • edited Loading

Codecov Report

VGalaxies commented Apr 15, 2024

imbajin left a comment

Choose a reason for hiding this comment

feat(pd): integrate `pd-core` into hugegraph #2478

feat(pd): integrate `pd-core` into hugegraph #2478

VGalaxies commented Mar 14, 2024 •

edited

Loading

codecov bot commented Mar 14, 2024 •

edited

Loading

VGalaxies commented Mar 17, 2024 •

edited

Loading

imbajin commented Mar 24, 2024 •

edited

Loading

codecov-commenter commented Apr 4, 2024 •

edited

Loading