Skip to content

fix(s3): keep multipart upload ID when abort after failed completion fails - #949

Closed
mayuriphad wants to merge 1 commit into
pyathena-dev:masterfrom
mayuriphad:fix/945-keep-upload-id-on-failed-abort
Closed

mayuriphad wants to merge 1 commit into
pyathena-dev:masterfrom
mayuriphad:fix/945-keep-upload-id-on-failed-abort

Conversation

@mayuriphad

Copy link
Copy Markdown

Refs #945.

WHAT

\S3File.commit()\ now keeps \multipart_upload\ and the part futures when the abort that follows a failed \CompleteMultipartUpload\ also fails. A successful abort still clears them, as before.

WHY

If the abort fails, the upload still exists. Clearing the upload ID meant a later \discard()\ (for example, a transaction rollback) had nothing to abort, so the incomplete upload kept accruing storage cost until a lifecycle rule removed it.

Implementation: _finish_multipart_upload()\ marks the exception it re-raises (\multipart_abort_failed) only when its abort fails. \commit()\ clears the upload state unless that marker is set.

TEST

  • New unit test \TestS3File::test_commit_failed_completion_upload_id\ (mocked client, no AWS): the abort-fails case fails on \master\ and passes with this change; the abort-succeeds case passes on both.
  • \ ests/pyathena/filesystem/test_s3.py\ existing \ est_discard\ cases: pass.

  • uff check\ and
    uff format --check\ pass on the changed files.
  • Ran with --noconftest\ so the session hook that uploads data to the project's AWS account did not run.
  • Not run: AWS integration tests and \just lint\ / mypy. This change affects only a failure path that the contribution guide allows to be tested with mocks, but I have not run it against real S3.

…fails

When CompleteMultipartUpload failed and the abort also failed,
S3File.commit() cleared the upload ID, so a later discard() had nothing
to abort and the incomplete upload stayed in S3 accruing storage cost.

The helper now marks the exception when the abort fails, and commit()
keeps the upload ID in that case so discard() can retry the abort.

Refs pyathena-dev#945
@laughingman7743

Copy link
Copy Markdown
Member

Thank you for looking into #945 and for the PR.
We have handled this issue in-house in #1047. It takes a different approach: commit() aborts the upload through discard(), which keeps the upload ID when the abort fails. So I'm closing this PR.
Thanks again for your interest in PyAthena.

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