[SVCS-552] folder_file_op cut down revalidate calls. #311

Johnetordoff · 2017-12-20T20:26:42Z

Ticket

https://openscience.atlassian.net/browse/SVCS-552
This PR replaces #287

Purpose

folder_file_op makes roughly a trillion metadata calls every time it moves a folder's children. This fix makes it easy to get a path from it's metadata object by giving the metadata an id which corresponds with the path identifier. Since we can now convert a metadata object into a path easily we no longer need to re-validate each time.

Changes

Add id property to every provider's metadata.
Changes from_metadata_to_path to uses metadata.id property
Changes folder_file op to use from_metadata_to_path instead of revalidate_path
Adds tests id property
Adds tests for folder_file_op

Side effects

None that I know of.

QA Notes

Copy/moves between providers should be unchanged or slightly faster.

Deployment Notes

None.

coveralls · 2017-12-20T22:17:21Z

Coverage increased (+0.4%) to 90.34% when pulling 6732325 on Johnetordoff:folder_file_op_fix into 1b60f40 on CenterForOpenScience:develop.

coveralls · 2018-01-02T22:03:39Z

Coverage increased (+0.4%) to 90.197% when pulling b61b2ec on Johnetordoff:folder_file_op_fix into 2f45e0b on CenterForOpenScience:develop.

cslzchen

Looks good and I will take over the ticket/PR. Two minor issues:

Need to update Google Cloud as well which has been added recently.
This change affects all path-based provider and will cause conflicts for sure. Proceed with caution 🚔 .

cslzchen · 2018-04-27T15:06:29Z

waterbutler/core/metadata.py


+    @property
+    @abc.abstractmethod
+    def id(self) -> str:


Please update the DocStr to mention what this id is for both types.

cslzchen · 2018-04-27T15:09:46Z

waterbutler/core/provider.py

        folder = await dest_provider.create_folder(dest_path, folder_precheck=False)

-        dest_path = await dest_provider.revalidate_path(dest_path.parent, dest_path.name, folder=dest_path.is_dir)
+        folder_path = dest_provider.path_from_metadata(dest_path.parent, folder)


Rename folder to folder_metadata (including the assign statement two lines above).

cslzchen · 2018-04-27T19:22:53Z

waterbutler/providers/bitbucket/metadata.py


+    @property
+    def id(self):
+        return self.build_path()


Since id is exactly the same, why not using return self.path? This is what other providers do.

- This id consolidates both path-based ID and id- based ID under a single property. - It is identical to the path object identifier which is used to revalidate paths.

- In addititon, remove superfluous condition statement `if not futures: continue`

- `self.id` is identical to `self.path` since GCS is a path-based provider

- Intermediate commit. Please rebase or ammend. - Discuss the todos and comments, fix issues if any. Update docstring, comments and remove todos. - Need travis ci.

cslzchen

@Johnetordoff and CC @felliott

I rebased and updated the code with extra comments for each provider affected. Need some further discussion with you before I can continue. Thanks.

cslzchen · 2018-05-30T21:56:15Z

waterbutler/providers/googlecloud/metadata.py

        return 'googlecloud'

+    @property
+    def id(self) -> str:


Add GoogleCloud to pass tests.

cslzchen · 2018-05-30T21:56:39Z

waterbutler/core/metadata.py


+    @property
+    @abc.abstractmethod
+    def id(self) -> str:


cslzchen · 2018-05-30T22:00:51Z

waterbutler/providers/googlecloud/metadata.py

        return 'googlecloud'

+    # GoogleCloud is a path-based provider.  However, it never calls ``revalidate_path()`` since it
+    # is the backend storage provider for OSFStorage.


Similar to CloudFiles (when it was the storage provider), GoogleCloud never uses revalidate_path(). We should set the id to be None or empty so we know it is never used accidentally or cause any confusion in the further.

cslzchen · 2018-05-30T22:03:13Z

waterbutler/providers/github/metadata.py

        return 'github'

+    # GitHub is a path-based provider. However, it has its own ``revalidate_path()``.
+    # TODO: Investigate if this is an issue.


GitHub may not work because your fix calls (functionally, not literally) the default revalidate_path() instead of the customized one.

cslzchen · 2018-05-30T22:04:06Z

waterbutler/providers/dropbox/metadata.py

        return 'dropbox'

+    # TODO: Is Dropbox path-based instead of id-based?
+    # TODO: Dropbox does not have a ``revalidate_path()``, should we use path instead of id?


There is no revalidate_path() for dropbox which makes guess that it is a path based (or at lease both)?

cslzchen · 2018-05-30T22:05:13Z

waterbutler/providers/dataverse/metadata.py


+    # TODO: Should this be in ``DataverseFileMetadata``?
+    # TODO: Does ``self.raw['id']`` exists for ``DataverseDatasetMetadata``?
+    # TODO: DataVerse's ``revalidate_path()`` looks very different. Is this an issue?


Similar to GitHub, your fix calls (functionally, not literally) revalidate_path() differently than the original way. This might be an issue.

cslzchen · 2018-05-30T22:05:25Z

waterbutler/providers/cloudfiles/metadata.py


+    # CloudFiles is a path-based provider.  However, it never called ``revalidate_path()`` even when
+    # it was the backend storage provider for OSFStorage.  The provider is no longer active until it
+    # is upgraded to an addon provider for the users.


See GoogleCloud.

cslzchen mentioned this pull request Jan 8, 2018

[SVCS-552] Remove unneeded API calls in folder_file_ops #287

Closed

cslzchen added the Code Review label Jan 8, 2018

cslzchen self-requested a review January 8, 2018 21:33

cslzchen reviewed Apr 27, 2018

View reviewed changes

cslzchen added Add'l Dev and removed Code Review labels Apr 27, 2018

Johnetordoff added 3 commits May 30, 2018 10:58

Add the id property to core and provider metadata

aaff576

- This id consolidates both path-based ID and id- based ID under a single property. - It is identical to the path object identifier which is used to revalidate paths.

Add/update tests/fixtures for metadata id property

bf5f9e3

Replace unnecessary path revalidation

a27b568

- In addititon, remove superfluous condition statement `if not futures: continue`

cslzchen force-pushed the folder_file_op_fix branch from 6732325 to a27b568 Compare May 30, 2018 15:10

cslzchen added 2 commits May 30, 2018 16:17

Add the id property to GoogleCloud metadata

63825aa

- `self.id` is identical to `self.path` since GCS is a path-based provider

Update docstring, comments and todos

b61b2ec

- Intermediate commit. Please rebase or ammend. - Discuss the todos and comments, fix issues if any. Update docstring, comments and remove todos. - Need travis ci.

cslzchen force-pushed the folder_file_op_fix branch from 8adfb5c to b61b2ec Compare May 30, 2018 21:55

cslzchen reviewed May 30, 2018

View reviewed changes

cslzchen added Needs ⛺🔥 and removed Add'l Dev labels Oct 12, 2018

[SVCS-552] folder_file_op cut down revalidate calls. #311

Are you sure you want to change the base?

[SVCS-552] folder_file_op cut down revalidate calls. #311

Uh oh!

Conversation

Johnetordoff commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

Purpose

Changes

Side effects

QA Notes

Deployment Notes

Uh oh!

coveralls commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jan 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cslzchen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cslzchen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Johnetordoff commented Dec 20, 2017 •

edited

Loading

coveralls commented Dec 20, 2017 •

edited

Loading

coveralls commented Jan 2, 2018 •

edited

Loading