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 bugs on Watcher and connect()/close(). #19

Closed
wants to merge 8 commits into from

Conversation

jpark-prestolabs
Copy link
Contributor

This fix includes,

  • Add metadata (auth token) to Watcher
  • Fix bugs on connect()/close() when multi-tasks are running at the same time
  • Add UnauthenticatedError
  • Remove decorators from some method to avoid repeated function calls
  • Apply black for the formatting.

@martyanov
Copy link
Owner

Hey @jpark-prestolabs , many thanks for the changes and continous updates. Unfortunately, I didn't have enough time to review the MR. I'll try to look at it over the weekend.

@martyanov
Copy link
Owner

@jpark-prestolabs if you don't mind I'll split the MR into several chunks and cherry-pick by one.

@jpark-prestolabs
Copy link
Contributor Author

@jpark-prestolabs if you don't mind I'll split the MR into several chunks and cherry-pick by one.

Thank you for your review. It's good to me.

@martyanov martyanov mentioned this pull request Jan 22, 2023
aetcd/rtypes.py Show resolved Hide resolved
aetcd/rtypes.py Show resolved Hide resolved
setup.py Show resolved Hide resolved
aetcd/client.py Show resolved Hide resolved
aetcd/client.py Show resolved Hide resolved
aetcd/client.py Show resolved Hide resolved
NOTE: It could be called while other operation is being executed.
At that time, the executing operation will be broken.
"""
for _ in range(MAX_COUNT_TO_WAIT_CONNECT_CHECK):
Copy link
Owner

Choose a reason for hiding this comment

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

If we already have _connection_check_timeout what is the reasoning behind MAX_COUNT_TO_WAIT_CONNECT_CHECK and multiple tries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's long time ago for me to update it. Sorry I cannot remember the exact reason.
Probably _connection_check_timeout could be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I remember that it is for etcd cluster.
MAX_COUNT_TO_WAIT_CONNECT_CHECK is used for making a connection.
One host name is used for multiple etcd instances in a cluster. So I'd like to make multiple trial.
But here, it could not be required.

Copy link
Owner

Choose a reason for hiding this comment

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

I've created an issue #20 to natively support multiple endpoints.

aetcd/client.py Show resolved Hide resolved
@martyanov
Copy link
Owner

I will mostly retain the old formatting as it is highly depends on tooling. The only way to fix it consistently is to intergrate black into the pipeline.

@@ -203,6 +271,20 @@ async def __aenter__(self):
async def __aexit__(self, *args):
await self.close()

@_handle_errors
@_ensure_connected
async def reset_auth_token(self) -> None:
Copy link
Owner

Choose a reason for hiding this comment

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

This probably should be refactored into private _auth and public auth and reused in connect.

@martyanov
Copy link
Owner

martyanov commented Feb 17, 2023

@jpark-prestolabs I cherry-picked everything except reset_auth_token. The commited changes are definitely need testing and then I'll create a release. Could you please create a separate MR for rest_auth_token? Please report any issues with the current master. Closing this MR for now.

@martyanov martyanov closed this Feb 17, 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.

3 participants