-
Notifications
You must be signed in to change notification settings - Fork 84
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
Fix/4285 limit disk on parallel upload #4329
Conversation
…ix/4285-limit-disk-on-parallel-upload
…odalab/codalab-worksheets into fix/4285-limit-disk-on-parallel-upload
… messing up testS
da91bb6
to
06b0f91
Compare
raise Exception( | ||
'Upload aborted. User disk quota exceeded. ' | ||
'To apply for more quota, please visit the following link: ' | ||
'https://codalab-worksheets.readthedocs.io/en/latest/FAQ/' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we refactor this link out into a constant?
@@ -114,7 +114,7 @@ def _make_request( | |||
raise RestClientException('Invalid JSON: ' + response_data, False) | |||
|
|||
def _upload_with_chunked_encoding( | |||
self, method, url, query_params, fileobj, progress_callback=None | |||
self, method, url, query_params, fileobj, pass_self=False, progress_callback=None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does pass_self mean? Can you document here and in the other function where you add it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
codalab/worker/upload_util.py
Outdated
@@ -32,6 +33,11 @@ def upload_with_chunked_encoding( | |||
:param url: String. Location or sub url that indicating where the file object will be uploaded. | |||
:param need_response: Bool. Whether need to wait for the response. | |||
:param progress_callback: Function. Callback function indicating upload progress. | |||
:param json_api_client: JsonApiClient. None when this function is run by the server. | |||
Used to update disk usage from client. | |||
:param check_disk: bool. True if the user's disk should be checked during the upload. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This param doesn't exist anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -61,16 +67,33 @@ def upload_with_chunked_encoding( | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the exact same fn as the one in upload_manager.py? If so we might want to refactor / unify in a future PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essentially. There are a few minor differences. I agree that it should be refactored.
codalab/worker/upload_util.py
Outdated
@@ -92,7 +115,10 @@ def upload_with_chunked_encoding( | |||
logging.debug("Socket timeout, retrying url: %s", url) | |||
conn.send(b'\0') | |||
logging.debug("Finished reading the response, url: %s", url) | |||
print("Finished reading the response, url: %s", url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove these print statements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
USER_READ_ONLY_FIELDS. We make this special function (which only allows | ||
positive disk increments so that users can't decrement their disk used) to ensure | ||
that we can safely increment user disk used without introducing a | ||
security flaw. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add documentation as to what the request format should look like (i.e., the disk_used_increment
key)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
tests/cli/test_cli.py
Outdated
_run_command([cl, 'work', worksheet_uuid]) | ||
# expect to fail when we upload something more than 2k bytes | ||
check_contains( | ||
'Upload aborted. User disk quota exceeded. ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just check if it contains "User disk quota exceeded" -- this way we don't need to change this test if we slightly modify the error message in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Co-authored-by: Ashwin Ramaswami <aramaswamis@gmail.com>
Reasons for making this change
We want to make sure users can't bypass their disk quota limits by uploading files in parallel
Related issues
Fixes #4285