Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add move to object client + change datasets.mv #163

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jkeelan
Copy link

@jkeelan jkeelan commented Apr 30, 2020

This PR adds move to the object client, using the object-move endpoint.

This reduces the number of API calls for datasets.mv. I've tested the functionality and it seems to work as expected.

@imrehg
Copy link
Member

imrehg commented May 1, 2020

Can this and #162 combined maybe? Or either way have to keep that other one in mind for this change too, right?

@jkeelan
Copy link
Author

jkeelan commented May 1, 2020

Yeah. Either combine or I'll merge once #162 hits master.

Copy link
Member

@acroz acroz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left a few comments.

An important thing to note is that this move endpoint works asynchronously - the lifetime of the request looks like:

  • Receive request
  • List all files under the source path
  • Work out all the above files' destination paths and dispatch individual move operations to a queue
  • Request completes
  • The service churns through the queue of operations to complete

At the very least, the fact that this operation is asynchronous should be well documented. We should also consider whether it is acceptable to change the behaviour of datasets.mv - users of this interface may rely on it blocking until complete in scripts etc.

Move object from this source path.
destination : str
Move object to this destination path.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: remove extra blank line

Comment on lines +270 to +271
if err.error_code == "source_path_not_found":
raise PathNotFound(source)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should re-raise the original exception if the error code does not match:

Suggested change
if err.error_code == "source_path_not_found":
raise PathNotFound(source)
if err.error_code == "source_path_not_found":
raise PathNotFound(source)
else:
raise

@@ -378,6 +378,28 @@ def test_object_client_copy_source_is_a_directory(mocker):
client.copy(PROJECT_ID, "source", "destination")


def test_object_client_move_url_encoding(mocker):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this also functions as the main test of ObjectClient.move, I suggest naming this:

Suggested change
def test_object_client_move_url_encoding(mocker):
def test_object_client_move(mocker):

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

Successfully merging this pull request may close these issues.

3 participants