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

fix: remove new exceptions #326

Merged
merged 6 commits into from
Mar 21, 2024
Merged

fix: remove new exceptions #326

merged 6 commits into from
Mar 21, 2024

Conversation

wey-gu
Copy link
Contributor

@wey-gu wey-gu commented Mar 21, 2024

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number: #325

@@ -274,15 +266,15 @@ def close(self):
self._connection._iprot.trans.close()
except Exception as e:
logger.error(
'Close connection to {}:{} failed:{}'.format(self._ip, self._port, e)
"Close connection to {}:{} failed:{}".format(self._ip, self._port, e)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

please ignore formatter's change, sorry

@@ -25,6 +25,7 @@ def __init__(
auth_result: AuthResult,
pool,
retry_connect=True,
retry_execute=False,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's make the execution error retry opt-in, thus to not surprise anyone.

@@ -35,7 +36,8 @@ def __init__(
:param auth_result: The result of the authentication process.
:param pool: The pool object where the session was created.
:param retry_connect: A boolean indicating whether to retry the connection if it fails.
:param retry_times: The number of times to retry the connection.
:param retry_execute: A boolean indicating whether to retry the execution if got execution error(-1005), by default False.
:param retry_times: The number of times to retry the connection/execution.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we don't need retry_execute param, 0 retry_times means no retry for execution. and mark retry_times default 0 to keep the same with no-retry mechanism.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree :), the retry times is already a newly introduced and it's feasible to be as flag with 0 value, done

@codecov-commenter
Copy link

codecov-commenter commented Mar 21, 2024

Codecov Report

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

Project coverage is 76.90%. Comparing base (bcc60e7) to head (e66b007).

Files Patch % Lines
nebula3/gclient/net/SessionPool.py 41.46% 24 Missing ⚠️
nebula3/gclient/net/Session.py 26.08% 17 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #326      +/-   ##
==========================================
- Coverage   77.23%   76.90%   -0.33%     
==========================================
  Files          18       18              
  Lines        2530     2516      -14     
==========================================
- Hits         1954     1935      -19     
- Misses        576      581       +5     

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

HarrisChu
HarrisChu previously approved these changes Mar 21, 2024
@Nicole00 Nicole00 merged commit c5d8ca7 into master Mar 21, 2024
10 checks passed
@Nicole00 Nicole00 deleted the remove_new_exceptions branch March 21, 2024 09:44
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.

None yet

4 participants