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

Revamp AWS file support to add support for multipart uploads in glacier testing #257

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

Conversation

netsettler
Copy link
Collaborator

  • In new module bucket_utils.py:

    • parse_s3_object_name
  • In common.py:

    • New glacier-related constants:

      • STANDARD
      • REDUCED_REDUNDANCY
      • STANDARD_IA
      • ONEZONE_IA
      • INTELLIGENT_TIERING
      • GLACIER
      • DEEP_ARCHIVE
      • OUTPOSTS
      • GLACIER_IR
    • New type hint S3ObjectNameSpec

  • In glacier_utils.py:

    • Allow a version_id= argument to GlacierUtils.is_restore_finished

    • Some improved error messages.

    • Some small code refactors.

  • In misc_utils.py:

    • Make make_counter threadsafe so that threaded functionality can call it.
  • In qa_utils.py:

    • Support for mock glacier testing in MockBotoS3Client for methods:

      • create_multipart_upload
      • upload_part_copy
      • complete_multipart_upload
    • Revamp the abstractions for managing MockFileSystem to allow for centralized changes that might be needed to handle new file content types, such as

      • MockAbstractContent

        • MockBigContent for mocking large files quickly and space-efficiently.

        • MockPartableBytes for mocking small content that still wants to test piecewise-copying in support of the multipart upload protocol.

Copy link
Member

@willronchetti willronchetti left a comment

Choose a reason for hiding this comment

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

This all looks good and should help the testing generally, just a few small comments

@@ -47,6 +52,13 @@
exported(QA_EXCEPTION_PATTERN, find_uses, confirm_no_uses, VersionChecker, ChangeLogChecker)


def make_unique_token(monotonic=False): # effectively a guid but for things that don't promise specifically a guid
Copy link
Member

Choose a reason for hiding this comment

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

Might be useful to include info on what the intended use of this is for?

pass


class MockPartableContent(MockAbstractContent):
Copy link
Member

Choose a reason for hiding this comment

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

Docstring should note partable == simulating AWS S3 multi part upload

class MockPartableContent(MockAbstractContent):

@classmethod
def start_cloning_from(cls, content):
Copy link
Member

Choose a reason for hiding this comment

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

Not clear what "cloning" means here?

Comment on lines +625 to +626
# We might at some future point want to consider whether callers of this function should
# see auto-mirroring or versioning.
Copy link
Member

Choose a reason for hiding this comment

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

These types of comments seem to be a mix of docstring vs. inline comments, may be worth generating docstrings for all of these and adding the notes where appropriate

return result

def restore_object(self, Bucket, Key, RestoreRequest, VersionId: Optional[str] = None,
StorageClass: Optional[S3StorageClass] = None):
duration_days: int = RestoreRequest.get('Days')
duration_days = RestoreRequest.get('Days')
# NOTE: Dcoumentation says "Days element is required for regular restores, and must not be provided
Copy link
Member

Choose a reason for hiding this comment

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

Small typo: Documentation

Comment on lines +2290 to +2292
# ====================
scenario(1)

Copy link
Member

Choose a reason for hiding this comment

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

Should these not be split into separate tests?

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

Successfully merging this pull request may close these issues.

2 participants