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

chore(pd-store-dev): integrate pd-grpc, pd-common, pd-client into hugegraph #2460

Merged
merged 18 commits into from
Mar 11, 2024

Conversation

VGalaxies
Copy link
Contributor

@VGalaxies VGalaxies commented Feb 25, 2024

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

For the pd-client submodule, its structure is as follows:

.
├── client
│  ├── AbstractClient.java // Encapsulation of RPC calls and error handling
│  ├── AbstractClientStubProxy.java // Management of multiple host stubs
│  ├── Channels.java // Maintenance of host -> ManagedChannel
│  ├── ClientCache.java // Cache for store, partition, shard group, graph
│  ├── Discoverable.java // unused
│  ├── DiscoveryClient.java // unused
│  ├── DiscoveryClientImpl.java // unused
│  ├── KvClient.java // Encapsulation of kv.proto RPC calls, inherits from AbstractClient
│  ├── LicenseClient.java // unused
│  ├── PDClient.java // Encapsulation of pdpb.proto RPC calls, encapsulates PDWatch
│  ├── PDConfig.java // Configuration of PD RPC port
│  ├── PDPulse.java // pd_pulse.proto, Partition heartbeat, Bidirectional communication interface of pd-client and pd-server
│  ├── PDPulseImpl.java
│  ├── PDWatch.java // pd_watch.proto, Listen for changes in store, partition, shard group, graph
│  └── PDWatchImpl.java
├── pulse
│  ├── PartitionNotice.java
│  └── PulseServerNotice.java
└── watch
   ├── NodeEvent.java // Store node event
   ├── PartitionEvent.java // Partition event
   ├── PDWatcher.java // unused
   └── WatchType.java // unused

TODO:

  • unit tests
  • code style
  • CJK characters (ignored)
  • dependency-review

@VGalaxies VGalaxies requested a review from imbajin February 25, 2024 07:19
@VGalaxies VGalaxies self-assigned this Feb 25, 2024
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. feature New feature labels Feb 25, 2024
@VGalaxies VGalaxies changed the title chore(pd-store-dev): integrate pd-grpc, pd-common, pd-client hugegraph chore(pd-store-dev): integrate pd-grpc, pd-common, pd-client into hugegraph Feb 25, 2024
Copy link

codecov bot commented Feb 25, 2024

Codecov Report

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

Project coverage is 63.23%. Comparing base (47a68f0) to head (19a7184).

Files Patch % Lines
...org/apache/hugegraph/pd/common/PartitionCache.java 0.00% 218 Missing ⚠️
.../java/org/apache/hugegraph/pd/common/HgAssert.java 0.00% 44 Missing ⚠️
...in/java/org/apache/hugegraph/pd/common/KVPair.java 0.00% 20 Missing ⚠️
...ava/org/apache/hugegraph/pd/common/GraphCache.java 0.00% 16 Missing ⚠️
...apache/hugegraph/pd/common/PDRuntimeException.java 0.00% 14 Missing ⚠️
...org/apache/hugegraph/pd/common/PartitionUtils.java 0.00% 14 Missing ⚠️
...va/org/apache/hugegraph/pd/common/PDException.java 0.00% 13 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                 @@
##             pd-store-dev    #2460      +/-   ##
==================================================
- Coverage           63.80%   63.23%   -0.57%     
+ Complexity            829      827       -2     
==================================================
  Files                 511      518       +7     
  Lines               42622    42959     +337     
  Branches             5947     5981      +34     
==================================================
- Hits                27193    27164      -29     
- Misses              12679    13040     +361     
- Partials             2750     2755       +5     

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


public String nextHost() {
String host = hostList.poll();
hostList.offer(host); //移到尾部
Copy link
Contributor

Choose a reason for hiding this comment

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

Translate comments into English.

Copy link
Member

Choose a reason for hiding this comment

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

Translate comments into English.

We will do it together in another PR (later), reduce (style)time cost in each PR (So as others)

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Mar 5, 2024
Comment on lines +51 to +57
private volatile Map<String, GraphCache> caches = new ConcurrentHashMap<>();

public ClientCache(org.apache.hugegraph.pd.client.PDClient pdClient) {
groups = new ConcurrentHashMap<>();
stores = new ConcurrentHashMap<>();
client = pdClient;
}
Copy link
Contributor

@Pengzna Pengzna Mar 11, 2024

Choose a reason for hiding this comment

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

Maybe we can consider to put the initialization of member variables all in the constructor for a cleaner code style. This can be fixed in the next style related PR. @VGalaxies

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can consider to put the initialization of member variables all in the constructor for a cleaner code style. This can be fixed in the next style related PR. @VGalaxies

Yep, we could mark a TODO first & enhance it with the v4.0 together

Comment on lines +305 to +306
<!-- enable it by default in ARM Mac to handle the compilation problems:) -->
<profile>
Copy link
Member

@imbajin imbajin Mar 11, 2024

Choose a reason for hiding this comment

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

enhance the building way here

Copy link
Member

@imbajin imbajin left a comment

Choose a reason for hiding this comment

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

PS: Historical issues will be optimized over time:)

@imbajin imbajin merged commit 3dd06f2 into pd-store-dev Mar 11, 2024
13 of 15 checks passed
@imbajin imbajin deleted the intro-pd-client branch March 11, 2024 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature lgtm This PR has been approved by a maintainer pd PD module size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants