Repository navigation
FsspecFileIO: Resolve the ADLS account per location instead of caching it in properties - #3886
krishnakaanchan-png wants to merge 2 commits into
Conversation
8c888a0 to
355a60d
Compare
…g it in properties _adls wrote the account name it inferred back into the properties dict it was given, and the caller passes the FileIO's own properties. The first location therefore pinned the account for every later one, so a second storage account was served a filesystem built for the first. Resolve the account and the SAS token into locals and leave the caller's dict alone. Precedence is unchanged: an explicit adls.account-name still wins, then a per account SAS token key, then the hostname.
355a60d to
27cea27
Compare
rambleraptor
left a comment
rambleraptor
left a comment
There was a problem hiding this comment.
This seems perfectly reasonable. We shouldn't alter the properties without a good reason.
It does look like the Java repository handles this differently than we do. The java repository uses the hostname to figure out which key to pass and we just blindly grab the first key.
I don't think we should necessarily fix this without some feedback from the community that it's a problem. I imagine many people only have one entry, so it ends up not being a problem.
| from azure.core.credentials_async import AsyncTokenCredential | ||
|
|
||
| for key, sas_token in { | ||
| # These are resolved into locals rather than written back into `properties`. The same |
There was a problem hiding this comment.
This comment doesn't seem necessary.
There was a problem hiding this comment.
Removed.
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Looks reasonable thanks @krishnakaanchan-png for working on this, and thanks @rambleraptor for the review 👍
This branch has not been deployed
3 participants
Closes #3885
Rationale for this change
_adlswrote the account name it inferred back into the properties dict it was handed, and the caller atfsspec.py:515passes the FileIO's ownself.properties. So the first ADLS location aFsspecFileIOtouched pinnedadls.account-namefor the life of that FileIO, and every later location was served a filesystem built for the first account.The
lru_cacheon(scheme, hostname)was doing its job. It built a second filesystem for the second hostname. By that pointadls.account-namewas already in the shared dict, so the inference was skipped and the new filesystem got the old account.Resolving into locals instead. The caller's dict is left alone, so the account is worked out again for each location.
Precedence is unchanged. An explicit
adls.account-namestill wins, then an account derived from a per account SAS token key, then the hostname.test_adls_account_name_sas_token_extractionpins that middle case and still passes.Same change for
adls.sas-token, which was being written back in the same loop.Are these changes tested?
Yes. Two new tests. One reads two locations in different accounts from a single FileIO and asserts each filesystem is built for its own account. The other asserts the properties dict is not touched at all.
Before, the reproduction in #3885 gave
['accountone', 'accountone']. Now it gives['accountone', 'accounttwo'].The three existing
_adlsaccount name tests still pass, including the SAS token one, which is the one that pins the precedence order.make lintis clean andmake testpasses.Are there any user-facing changes?
Yes, for anyone reading tables that span more than one storage account through
FsspecFileIOwithout settingadls.account-name. Those locations were being read from the wrong account and will now go to the right one. Single account setups behave the same as before.A FileIO's
propertiesalso no longer changes as a side effect of reading a location.