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

Why retrying on execution failure #325

Open
wenhaocs opened this issue Mar 19, 2024 · 4 comments
Open

Why retrying on execution failure #325

wenhaocs opened this issue Mar 19, 2024 · 4 comments
Labels
affects/none PR/issue: this bug affects none version. severity/none Severity of bug type/bug Type: something is unexpected

Comments

@wenhaocs
Copy link

wenhaocs commented Mar 19, 2024

Why do we retry on execution failure while nebula-go only retries on session invalid? Moreover, this feature is causing our nebula pytest failure, which (1) uses the latest nebula-python and (2) expects failures.

Related PR: #306

@wenhaocs wenhaocs added the type/bug Type: something is unexpected label Mar 19, 2024
@github-actions github-actions bot added affects/none PR/issue: this bug affects none version. severity/none Severity of bug labels Mar 19, 2024
@wey-gu
Copy link
Contributor

wey-gu commented Mar 20, 2024

The motivation was #276 && to make SDK with better DX, to enable retrying only when it makes sense and doesn't bring mess.

If the change is applied to the above purpose we could add options(for now, only one general retry flag was defined) to disable that opinionated retry to enable the pytest run as is? (And we will apply equivalent changes to other clients, too)

If not, I can revert the change.

@wenhaocs
Copy link
Author

Maybe we need to discuss whether we should throw the exception or return the error code.
In the kernel pytest, we are expecting error code to be returned: https://github.com/vesoft-inc/nebula-ent/blob/master/tests/job/test_session.py#L156. If you folks decide that throwing an exception is the right method, we need to revise the kernel pytest and wrap the related code in the try block.

@wey-gu
Copy link
Contributor

wey-gu commented Mar 20, 2024

An attempt to implement kernal pytest was done via https://github.com/vesoft-inc/nebula-ent/pull/3485 (I didn't run it in a fine-grained way locally at first, so it took me some time waiting for CIs, and surprisingly after two round of changes to fix 6 errors, another 10 errors came out),while after all py test passed, it turned out there is a big assumption in infra of tck...

It seems quite obvious that we'd better keep the retry mechanism in the session/session pool(not impacting tests actually) w/o introducing connection level exception.

Let's talk tomorrow.

@wey-gu wey-gu mentioned this issue Mar 21, 2024
3 tasks
@wey-gu
Copy link
Contributor

wey-gu commented Mar 21, 2024

will be fixed with #326 , where:

  • no new exceptions introduced
  • retry of execution error is opt-in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects/none PR/issue: this bug affects none version. severity/none Severity of bug type/bug Type: something is unexpected
Projects
None yet
Development

No branches or pull requests

2 participants