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

[GIE Compiler] Unify execution client which is used to send request to remote engine service #2818

Merged
merged 17 commits into from
Jun 7, 2023

Conversation

shirly121
Copy link
Collaborator

@shirly121 shirly121 commented Jun 6, 2023

What do these changes do?

as titled.

Related issue number

Fixes

@shirly121 shirly121 requested a review from longbinlai June 6, 2023 01:55
@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2023

Codecov Report

Merging #2818 (77d9499) into main (e28faa9) will decrease coverage by 31.48%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #2818       +/-   ##
===========================================
- Coverage   73.91%   42.44%   -31.48%     
===========================================
  Files          99       99               
  Lines       10665    10654       -11     
===========================================
- Hits         7883     4522     -3361     
- Misses       2782     6132     +3350     

see 57 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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


message HighQPSQuery {
common.NameOrId query_name = 1;
repeated Argument arguments = 2;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Donot call them HighQPS xx. These are StoredProcedureQuery, StoredProcedureResults and StoredProcedureArgument. The name is also stored_procedure.proto

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -112,7 +99,7 @@ public static void main(String[] args) throws Exception {
List<RpcChannel> channels = new ArrayList<>();
channels.add(rpcChannel0);
channels.add(rpcChannel1);
RpcClient rpcClient = new RpcClient(channels);
RpcClient rpcClient = new RpcClient(600000, channels);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can the RPC timeout be configured? What is the relation of this timeout with the Gremlin query timeout and Pegasus execution timeout?

Copy link
Collaborator Author

@shirly121 shirly121 Jun 6, 2023

Choose a reason for hiding this comment

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

现在RpcClient的timeout都是从配置里读的,这个是测试文件,所以直接给了个值;配置https://github.com/alibaba/GraphScope/pull/2818/files#diff-6cfeda4b115b8cce14e597b6798de6b7ee897f0b6ac69d14a00dc1013796c639

和gremlin/pegasus timeout是独立的配置项目,理论上需要gremlin timeout > grpc timeout > pegasus timeout


package com.alibaba.graphscope.common.config;

public class HQPSConfig {
Copy link
Collaborator

@longbinlai longbinlai Jun 7, 2023

Choose a reason for hiding this comment

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

Call this HiactorConfig. Correspondingly, the following configuration should start with HIACTOR_XX, and hiactor.xx.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done


public class HQPSConfig {
public static final Config<String> HQPS_URIS =
Config.stringConfig("hqps.uris", "http://localhost:8080");
Copy link
Collaborator

@longbinlai longbinlai Jun 7, 2023

Choose a reason for hiding this comment

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

Configure this as hosts: localhost:8080, just like Pegasus, and then we make it uris internally (this may change to grpc in the future, FYI. )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@shirly121 shirly121 requested a review from longbinlai June 7, 2023 03:47
Copy link
Collaborator

@longbinlai longbinlai left a comment

Choose a reason for hiding this comment

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

Lgtm

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