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 Gremlin Timeout Configurations #2953

Merged
merged 5 commits into from
Jul 3, 2023

Conversation

shirly121
Copy link
Collaborator

@shirly121 shirly121 commented Jun 30, 2023

What do these changes do?

Unify gremlin timeout configurations. By this pr, the console will output like this if gremlin query execution timeout is set to 2000ms.

image

Timeout can be set by two ways:

  1. system configuration, set query.execution.timeout.ms: 2000 in conf/ir.compiler.properties
  2. set per query in gremlin query g.with(ARGS_EVAL_TIMEOUT, 2000).V() or g.with(Tokens.ARGS_EVAL_TIMEOUT, 2000).V(). if timeout value is of long type, suffix with 'L'

Related issue number

Fixes #2854

@shirly121 shirly121 changed the title [GIE Compiler] Unify Gremlin Timeout Configuration [GIE Compiler] Unify Gremlin Timeout Configurations Jun 30, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jun 30, 2023

Codecov Report

Merging #2953 (e40ee8d) into main (7fc14b5) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2953   +/-   ##
=======================================
  Coverage   42.37%   42.37%           
=======================================
  Files          99       99           
  Lines       10649    10649           
=======================================
  Hits         4512     4512           
  Misses       6137     6137           

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 7fc14b5...e40ee8d. Read the comment docs.

@longbinlai
Copy link
Collaborator

longbinlai commented Jun 30, 2023

Set as 2s, but timeout shows 1.79s. Should we ignore the 1.79s in the output?

public QueryTimeoutConfig(long executionTimeoutMS) {
this.executionTimeoutMS = executionTimeoutMS;
this.channelTimeoutMS = (long) (executionTimeoutMS * (1 - GRADUAL_FACTOR));
this.engineTimeoutMS = (long) (executionTimeoutMS * (1 - 2 * GRADUAL_FACTOR));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make the three time align. Should be fine.

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

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.

@lidongze0629 Try these code on gs-portal?

@lidongze0629
Copy link
Collaborator

@lidongze0629 Try these code on gs-portal?

It seems fine with the gremlin nodejs client.

截屏2023-07-03 上午11 30 08

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

Copy link
Collaborator

@lidongze0629 lidongze0629 left a comment

Choose a reason for hiding this comment

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

Nice Job!

@longbinlai longbinlai merged commit 1e2c8f6 into alibaba:main Jul 3, 2023
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.

[GIE/Compiler] Unify all timeout configurations to a single query timeout
4 participants