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.

Sign up for GitHub

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

Jump to bottom

Add ListBucketVersions and improve S3.deleteBucketContents #2747

Merged
merged 1 commit into from
Oct 11, 2021
Merged

Add ListBucketVersions and improve S3.deleteBucketContents #2747

merged 1 commit into from
Oct 11, 2021

Conversation

mdedetrich
Copy link
Contributor

@mdedetrich mdedetrich commented Oct 8, 2021 •
edited
Loading

This PR adds the listBucketVersions API and also improves the S3.deteBucketContents so that it also deletes all version objects (previously if you had versioned objects then the S3.deleteBucketContents wouldn't properly clear the bucket which means that a consequent S3.deleteBucket would fail). This also mirrors S3's official documentation on how to clear a bucket, i.e. see https://docs.aws.amazon.com/AmazonS3/latest/userguide/delete-bucket.html.

There were also some minor documentation errors with listMultipartUploadAndCommonPrefixes which I fixed.

mdedetrich
mdedetrich commented Oct 8, 2021
View reviewed changes
s3/src/test/scala/akka/stream/alpakka/s3/scaladsl/S3IntegrationSpec.scala
@@ -155,7 +157,7 @@ trait S3IntegrationSpec
(put, delete, metaBefore, metaAfter)
}

val (putResult, deleteResult, metaBefore, metaAfter) = result.futureValue
val (putResult, _, metaBefore, metaAfter) = result.futureValue
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minor thing, I noticed that this variable wasn't being used.

@mdedetrich
Copy link
Contributor Author

mdedetrich commented Oct 8, 2021 •
edited
Loading

So the tests are currently failing with due to the command for enabling versioning i.e.

aws --endpoint-url http://minio:9000 s3api put-bucket-versioning --bucket my-bucket-with-versioning --versioning-configuration Status=Enabled

Fails locally with

An error occurred (NotImplemented) when calling the PutBucketVersioning operation: A header you provided implies functionality that is not implemented

When running against Minio. I don't know why this is happening, if you run the test against a real S3 then it works.

I am going to disable the test in Minio for now until I figure out how ti fix it.

EDIT: May have been due to a key collision, trying again

@mdedetrich mdedetrich force-pushed the add-listbucketversions-and-improve-deleteBucketContents branch 3 times, most recently from 699ba77 to 76bece7 Compare October 8, 2021 12:05
@mdedetrich
Copy link
Contributor Author

Okay so it seems the minio that is used in Travis doesn't really support versioning so I had to disable the test

@mdedetrich
Copy link
Contributor Author

Regarding this issue with not being able to make a versioned bucket in Minio to test properly, should I make an issue for it and annotate it as a comment in the code to fix it later?

@johanandren
Copy link
Member

Note and issue sounds good to me.

@mdedetrich mdedetrich force-pushed the add-listbucketversions-and-improve-deleteBucketContents branch from 76bece7 to d81b2a4 Compare October 11, 2021 09:22
mdedetrich
mdedetrich commented Oct 11, 2021
View reviewed changes
s3/src/main/scala/akka/stream/alpakka/s3/impl/S3Stream.scala
@@ -180,7 +213,7 @@ import scala.util.{Failure, Success, Try}
.mapAsync(parallelism = 1)(entityForSuccess)
.map {
case (entity, headers) =>
Option((entity.dataBytes.mapMaterializedValue(_ => NotUsed), computeMetaData(headers, entity)))
Some((entity.dataBytes.mapMaterializedValue(_ => NotUsed), computeMetaData(headers, entity)))
Copy link
Contributor Author

@mdedetrich mdedetrich Oct 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Another minor thing I noticed, this can never be null since we are using Scala idiomatic akka-http API so its safe to use Some here.

Copy link
Member

Choose a reason for hiding this comment

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

Also it is always handed a tuple regardless of the nullness of other other things :)

@mdedetrich
Copy link
Contributor Author

@johanandren Done

johanandren
johanandren approved these changes Oct 11, 2021
View reviewed changes
Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

LGTM but I think it could be slightly simplified without the fromMaterializer operator.

s3/src/main/scala/akka/stream/alpakka/s3/impl/S3Stream.scala
@@ -180,7 +213,7 @@ import scala.util.{Failure, Success, Try}
.mapAsync(parallelism = 1)(entityForSuccess)
.map {
case (entity, headers) =>
Option((entity.dataBytes.mapMaterializedValue(_ => NotUsed), computeMetaData(headers, entity)))
Some((entity.dataBytes.mapMaterializedValue(_ => NotUsed), computeMetaData(headers, entity)))
Copy link
Member

Choose a reason for hiding this comment

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

Also it is always handed a tuple regardless of the nullness of other other things :)

s3/src/main/scala/akka/stream/alpakka/s3/impl/S3Stream.scala
)

Source
.fromMaterializer { (mat, attr) =>
Copy link
Member

Choose a reason for hiding this comment

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

Why is the fromMaterializer/defered materialization needed? We already have an implicit Materializer in scope and the attributes doesn't look like they are used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I was just copying the style that is in this file, i.e. if you look at

You can see that this pattern is used throughout. I personally don't have a problem in simplifying this but maybe it makes sense to do it in a future PR so its consistent?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, didn't notice. Sure, perhaps there is a non-obvious reason wanting to defer that materialization that I don't know about, so let's go with what's already there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ennru Maybe you can comment on this, according to git blame Source.setup was used before it got changed to Source.fromMaterializer?

Copy link
Member

Choose a reason for hiding this comment

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

setup was mainly a predecessor to fromMaterializer. In many places we do this to access stream attributes, not sure what was the original reason here.

@johanandren johanandren merged commit 995b04c into akka:master Oct 11, 2021
@johanandren johanandren added this to the 3.0.4 milestone Oct 11, 2021
@mdedetrich mdedetrich deleted the add-listbucketversions-and-improve-deleteBucketContents branch October 11, 2021 11:08
This was referenced Oct 11, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ennru ennru ennru left review comments

@johanandren johanandren johanandren approved these changes

Assignees
No one assigned
Labels
p:aws-s3
Projects
None yet
Milestone
3.0.4
Development

Successfully merging this pull request may close these issues.
3 participants
@mdedetrich @johanandren @ennru