[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

@@ -133,6 +133,16 @@ def kind(self) -> str:
        """ `file` or `folder` """
        raise NotImplementedError

+    @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

@@ -334,7 +334,7 @@ def request(self, *args, **kwargs):

        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

@@ -24,6 +24,10 @@ def __init__(self, raw, path_obj, owner=None, repo=None):
        self.owner = owner
        self.repo = repo

+    @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

@@ -27,6 +27,10 @@ class BaseGoogleCloudMetadata(metadata.BaseMetadata, metaclass=abc.ABCMeta):
    def provider(self) -> str:
        return 'googlecloud'

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


Add GoogleCloud to pass tests.

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

waterbutler/core/metadata.py

@@ -133,6 +133,16 @@ def kind(self) -> str:
        """ `file` or `folder` """
        raise NotImplementedError

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


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

waterbutler/providers/googlecloud/metadata.py

@@ -27,8 +27,11 @@ class BaseGoogleCloudMetadata(metadata.BaseMetadata, metaclass=abc.ABCMeta):
    def provider(self) -> str:
        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

    @property
    def provider(self):
        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

    @property
    def provider(self):
        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

    @property
    def provider(self):
        return 'dataverse'

+    # 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

    @property
    def provider(self):
        return 'cloudfiles'

+    # 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Johnetordoff commented Dec 20, 2017 •

edited

Loading

coveralls commented Dec 20, 2017 •

edited

Loading

coveralls commented Jan 2, 2018 •

edited

Loading

cslzchen left a comment

cslzchen Apr 27, 2018

cslzchen May 30, 2018

cslzchen Apr 27, 2018

cslzchen Apr 27, 2018

cslzchen left a comment

cslzchen May 30, 2018

cslzchen May 30, 2018

cslzchen May 30, 2018

cslzchen May 30, 2018

cslzchen May 30, 2018

cslzchen May 30, 2018

cslzchen May 30, 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

Conversation

Johnetordoff commented Dec 20, 2017 • edited Loading

Ticket

Purpose

Changes

Side effects

QA Notes

Deployment Notes

coveralls commented Dec 20, 2017 • edited Loading

coveralls commented Jan 2, 2018 • edited Loading

cslzchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cslzchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Johnetordoff commented Dec 20, 2017 •

edited

Loading

coveralls commented Dec 20, 2017 •

edited

Loading

coveralls commented Jan 2, 2018 •

edited

Loading