Refactor do_connect for better logging #504
Conversation
And consistent ordering of credentials
|
There are now a few PRs waiting in this repo - please do ping me when you think any of them is ready to be merged. |
|
I think the test failures are due to Azurite: I've started a discussion here #505 |
|
Hi @jamesmyatt. Thanks for the PR! Could you elaborate more on the primary motivation for the PR? It to just be able to better trace how credentials are being resolved when using adlfs? Or is it more to make consolidate some of the client creation logic to make it more manageable for subsequent adlfs feature work? The main reason that I ask is there is another in-flight PR: #501 where we are adding some additional client configuration and doing some refactoring to at least consolidate how client keyword arguments are set. And it seems there is some overlap in trying to refactor/consolidate these. So, I'm just trying to figure out what is the best approach to incorporating both of these. I do agree there is some opportunities to consolidate logic even past what is done in #501 that would be beneficial for future development. |
|
Mostly it's to get better traceability/transparency over what credentials ADLFS is using to connect to storage, and to match the docstrings with the code. For example, I was surprised when it was using a connection string from os.environ when I wasn't expecting it to and I thought that better logging was the solution. TBH, I prefer this refactoring to #501, especially since it also eliminates more duplication, and it should be easy enough to combine the updates. But if the logging is clear, then I'll be happy since I'm not a maintainer here. So any of the following actions are OK for me:
|
|
@jamesmyatt Thanks for the explanation! That makes sense to me. I do like the direction of this PR's refactoring. I see the refactoring here as the next step in terms of building on what is being done in: #501. The other PR did not go to that length of refactoring in order to balance minimizing changes needed for the functionality while still cleaning up the logic. In terms of the options, I prefer this one:
Specifically, I see the changes in #501 as a steppingstone to these changes. For example, one thing that we realized was that there were not really any tests that leveraged non-connection string setups so #501 has been setting up some of the scaffolding needed to be able to assert how clients are constructed, which we can leverage for cases in this PR to ensure the refactoring maintains behavior. That being said, while progress toward merging in #501 is being made, I can take a first review of this PR to help minimize the number of update cycles needed to get this PR merged. |
|
That sounds great. Thanks |
| @@ -350,6 +350,13 @@ def __init__( | |||
| max_concurrency = batch_size | |||
| self.max_concurrency = max_concurrency | |||
|
|
|||
| @property | |||
| def account_url(self) -> str: | |||
There was a problem hiding this comment.
Technically, to keep the existing behavior, we will need to check whether the connection string is set and if so return None. We should add that since if a connection_string is provided, an account_name would not be and we could end up encoding None into the account url.
While it is possible to piece together an account url from a connection string, that would be quite a bit of logic for the purpose of this PR.
| @@ -521,6 +488,46 @@ def do_connect(self): | |||
| except Exception as e: | |||
| raise ValueError(f"unable to connect to account for {e}") from e | |||
|
|
|||
| def _get_service_client(self) -> AIOBlobServiceClient: | |||
There was a problem hiding this comment.
Could we hoist this helper internal method to the module level instead of making it a method on the class? Mainly that helps better avoid accessing internal methods of one class from another class.
| if not self.sas_token.startswith("?"): | ||
| self.sas_token = f"?{self.sas_token}" |
There was a problem hiding this comment.
It would be interesting if we could hoist this logic to the __init__. Mainly, going over this method now this is the last place where any mutation is happening on the file system. So if we move it, it give the nice property that this method does not mutate state.
| self.sas_token = f"?{self.sas_token}" | ||
| kwargs["account_url"] = f'{kwargs["account_url"]}{self.sas_token}' | ||
| else: | ||
| logger.info("Connect using anonymous login") |
There was a problem hiding this comment.
In the previous logic, the location_mode was never used in anonymous mode, meaning if a value of secondary was set adlfs would still use the primary endpoint. I'd lean toward retaining that behavior for now since the PR is more about refactoring and logging. But I'm open to respecting location_mode for anonymous connections if it supported for GRS accounts but would need to double check to confirm if it is.
| creds = ["credential", "sync_credential", "account_key"] | ||
| for name in creds: | ||
| if (cred := getattr(self, name, None)) is not None: | ||
| logger.info("Connect using %s", name.replace("_", " ")) |
There was a problem hiding this comment.
For these logging statements, let's set the level to debug. Mainly debug is the prevalent level used in the codebase and these statements are intended for debugging which credentials are used.
| "credential": None, | ||
| "_location_mode": self.location_mode, | ||
| } | ||
| creds = ["credential", "sync_credential", "account_key"] |
There was a problem hiding this comment.
Any ideas on why the file system's client and the file-object's client were out of sync in terms of credential resolution here in the first place? I need to go through the git history on how these became out of sync, but I'd probably lean toward even removing sync_credential from that list so that:
- We have complete parity with the
do_connect()method. Typically this is the method that most people will use to authenticate since by default even in theAzureBlobFileclass, it will just reuse the client from the file system unlessconnect_client()is explicitly called. - While technically it is possible to provide a sync credential to an async client, it does not seem like a practice that we should be continuing, instead async clients should be using async credentials.
I'll look more into this and circle back on what we do here.
Also consistent ordering of credentials throughout.