FsspecFileIO: Resolve the ADLS account per location instead of caching it in properties - #3886
Open
krishnakaanchan-png wants to merge 1 commit into
Open
Conversation
…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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.