Skip to content

gs: use 3-char prefixes - #6759

Merged
pmrowla merged 1 commit into
masterfrom
gs-unset-traverse-prefix
Oct 11, 2021
Merged

gs: use 3-char prefixes#6759
pmrowla merged 1 commit into
masterfrom
gs-unset-traverse-prefix

Conversation

@isidentical

@isidentical isidentical commented Oct 7, 2021

Copy link
Copy Markdown
Contributor

Resolves #6691. Normally it should have inherited this from the ObjectFSWrapper, but this seems like a leftover.

Resolves #6691. Normally it should have inherited this from the `ObjectFSWrapper`, but this seems like a leftover.
@isidentical
isidentical requested a review from a team as a code owner October 7, 2021 15:40
@isidentical
isidentical requested review from efiop, pared and pmrowla and removed request for pared October 7, 2021 15:40
@shcheklein

Copy link
Copy Markdown
Contributor

Quick question/suggestion - is there a way/should we to add a test for this?

@isidentical

Copy link
Copy Markdown
Contributor Author

Quick question/suggestion - is there a way/should we to add a test for this?

We already have 2 tests for this, but they cover the other way around.
https://github.com/iterative/dvc/blob/dd86076ba6a5ab882de4b3b68f8d42ec5b4ad971/tests/func/test_data_cloud.py#L573-L610

@efiop

efiop commented Oct 7, 2021

Copy link
Copy Markdown
Contributor

Very interesting that this broke because of 2, as I would expect that to be less efficient but still work 🤔 @pmrowla do we have some logic flaw or am I missing something?

On a side note, once we migrate to fs.find I guess we could default to searching for 00/0* by default (non-obj-clouds will just search 00/ anyway) to simplify the logic.

@pmrowla

pmrowla commented Oct 11, 2021

Copy link
Copy Markdown
Contributor

Very interesting that this broke because of 2, as I would expect that to be less efficient but still work 🤔 @pmrowla do we have some logic flaw or am I missing something?

It seems like there is probably still some bug in the gcsfs prefix implementation somewhere (that doesn't like prefix len 2) rather than this being on DVC's end.

@pmrowla
pmrowla merged commit 8767f42 into master Oct 11, 2021
@pmrowla
pmrowla deleted the gs-unset-traverse-prefix branch October 11, 2021 07:16
@efiop

efiop commented Oct 11, 2021

Copy link
Copy Markdown
Contributor

@pmrowla Was hoping to include a test here, strange that this hasn't been caught by our suit. Or do you think it should be tested on gcsfs side?

@pmrowla

pmrowla commented Oct 11, 2021

Copy link
Copy Markdown
Contributor

I think it needs to be tested on the gcsfs side. We use a mix of both 2 and 3 as prefix lengths on all of our other filesystems without seeing this issue.

@efiop efiop added the bugfix fixes bug label Oct 11, 2021
@nikitaorlovpicsart

Copy link
Copy Markdown

after update start seening this issue on random files

@efiop

efiop commented Oct 19, 2021

Copy link
Copy Markdown
Contributor

@nikitaorlovpicsart , could you elaborate?

@nikitaorlovpicsart

Copy link
Copy Markdown
  1. we use gs as remote
  2. we use most recent release
  3. issue mentioned in issue is still same for us, if we pull file directly, it works, but when we call pull in root folder, we get fail.

@nikitaorlovpicsart

Copy link
Copy Markdown

Every time all files start with 00

@efiop

efiop commented Oct 22, 2021

Copy link
Copy Markdown
Contributor

@nikitaorlovpicsart Could you show full verbose (-v) error you are getting? Also dvc doctor output, please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix fixes bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dvc pull returns "failed to pull data" when the data exists on remote

5 participants