Add s3 file ingestion sample to examples source tree for M1 release. #123

kggilmer · 2021-05-07T02:43:22Z

Issue #, if available: smithy-lang/smithy-kotlin#306

Description of changes:

This PR adds a sample app to demonstrate S3 functionality. It continues on the theme of movie media files. In this case it is a simple media ingestion job that demonstrates the following capabilities of the SDK:

Create S3 client, operations: listBuckets, listObjects, putObject
Showcases async nature of operations by using Kotlin flow for processing

Testing done

Verified that "media" files are uploaded to S3 as expected
Verified that S3 bucket check works
Verified that key collision check works

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

aajtodd

I like the example overall. I have two suggestions/concerns:

Having actual media files (.avi) may hinder this example. Of course a user could just create fake .avi files but I'm wondering how to make it as easy as possible to grok and try out.
I like the example but I definitely think we need to demonstrate getObject() (either in this example or in a different one) because it has the scoped response. Perhaps a separate, non-media project example would suffice? Something like a "round-tripped" example that just simply creates, uploads, lists, and re-downloads the result? In particular demonstrating or adding some comments around the ways ByteStream can be created or consumed (e.g. ByteStream.fromString(), fromBytes(), fromFile() and toByteArray(), decodeToString(), writeToFile().

aajtodd · 2021-05-07T10:07:08Z

examples/s3-media-ingestion/build.gradle.kts

+
+dependencies {
+    implementation(kotlin("stdlib"))
+    implementation("org.jetbrains.kotlinx:kotlinx-coroutines-core:1.4.3")


suggestion

probably move the coroutines version to the root examples project so that we only have to change in one place for all examples?

Sure but how? I tried a few obvious variations, looked in our source tree and did some searching but didn't find anything that worked. Can you point me to an example?

aajtodd · 2021-05-07T10:08:08Z

examples/s3-media-ingestion/src/main/kotlin/Main.kt

+import java.nio.file.Files
+
+const val bucketName = "s3-media-ingestion-example"
+const val ingestionDirPath = "/tmp/media-in"


question

are these meant to be changed by the user? These paths are *unix specific.

I added some notes in the file

aajtodd · 2021-05-07T10:09:23Z

examples/s3-media-ingestion/src/main/kotlin/Main.kt

+}
+
+// Check for valid S3 configuration based on account
+suspend fun validateS3(s3Client: S3Client) {


suggestion

validateBucketExists(...) or even change it to ensureBucketExists(..) and showcase a simple create bucket?

Updated to create bucket if not exists, good idea.

aajtodd · 2021-05-07T10:12:09Z

examples/s3-media-ingestion/src/main/kotlin/Main.kt

+        .listObjects(ListObjectsRequest { bucket = bucketName })
+        .contents?.any { it.key == mediaMetadata.s3KeyName } ?: false
+
+    if (existsInS3) return Failure("${mediaMetadata.s3KeyName} already uploaded.", mediaMetadata)


question

should this be a failure? I could see it being useful to just log a statement but otherwise succeed. Or just overwrite?

"Failure" may be the wrong name but I like the idea of this method returning multiple statuses, including one for "this already exists, no-op".

renamed to FileExistsError

But yeah my notion for adding this was simply to have more idomatic kotlin (sealed class types to represent result states), but it's not super tied to S3 itself.

ianbotsf · 2021-05-07T15:24:57Z

examples/s3-media-ingestion/src/main/kotlin/Main.kt

+// Check for valid S3 configuration based on account
+suspend fun validateS3(s3Client: S3Client) {


Nit: Prefer KDoc over plain comments:

/** Check for valid S3 configuration based on account */ suspend fun validateS3(s3Client: S3Client) {

Applies to multiple functions in this file.

ianbotsf · 2021-05-07T15:52:16Z

examples/s3-media-ingestion/src/main/kotlin/Main.kt

+// Upload to S3 if file not already uploaded
+suspend fun uploadToS3(s3Client: S3Client, mediaMetadata: MediaMetadata): UploadResult {
+    val existsInS3 = s3Client
+        .listObjects(ListObjectsRequest { bucket = bucketName })
+        .contents?.any { it.key == mediaMetadata.s3KeyName } ?: false


Correctness: Listing files has a few problems:

S3 recommends no longer using ListObjects in favor of ListObjectsV2

There's no pagination here. When the bucket has a lot of files, this will yield an incorrect outcome if the expected file isn't found in the first page of results.

Listing resources when we want exactly one by primary key is generally wasteful. It's generally more costly (in this case ~10x) and less efficient.

Instead, how about a HEAD request. Unvalidated code sample:

val existsInS3 = try { s3Client.headObject( HeadObjectRequest { bucket = bucketName key = mediaMetadata.s3KeyName } ) true } catch (e: NotFoundException) { false }

Nice, great idea.

FWIW this uncovers one area where our SDK is not yet ready...error customizations. The s3 client should throw a NotFoundException but it does not at this time because we don't have a customized error handler for S3.

What does it do instead?

it throws UnknownServiceErrorException because here we fail to deserialize anything from the response.

Well then I'll leave it up to you. Maybe calling listObjects is a better example since it's semantically clear.

No I prefer the head calls. More efficient.

I made a note in the code that the exception catching will change in a subsequent SDK release

ianbotsf · 2021-05-07T15:58:49Z

examples/s3-media-ingestion/src/main/kotlin/Main.kt

+        .listObjects(ListObjectsRequest { bucket = bucketName })
+        .contents?.any { it.key == mediaMetadata.s3KeyName } ?: false
+
+    if (existsInS3) return Failure("${mediaMetadata.s3KeyName} already uploaded.", mediaMetadata)


"Failure" may be the wrong name but I like the idea of this method returning multiple statuses, including one for "this already exists, no-op".

ianbotsf · 2021-05-07T15:59:46Z

examples/s3-media-ingestion/src/main/kotlin/Main.kt

+        val uploadResults = ingestionDir
+            .walk().asFlow()
+            .mapNotNull(::mediaMetadataExtractor)
+            .map { mediaMetadata ->
+                uploadToS3(client, mediaMetadata)
+            }
+            .toList()


Style: Generally I find it cleaner when using multiline fluent calls to keep each method call in the chain on a separate line. That is, either:

val x = obj.doA().doB().doC()

Or:

val x = obj .doA() .doB() .doC()

But not:

val x = obj .doA().doB() .doC()

In this case, .asFlow() might be better on a dedicated line.

Fixed, thanks

Add s3 file ingestion sample to examples source tree for M1 release.

c3fd97e

kggilmer requested review from aajtodd and ianbotsf May 7, 2021 02:43

aajtodd approved these changes May 7, 2021

View reviewed changes

ianbotsf reviewed May 7, 2021

View reviewed changes

kggilmer and others added 5 commits May 7, 2021 14:54

Updates based on PR feedback.

af5fac4

Tweaks to comments and constants

9f198c9

Another tweak to avoid invalid log message in error case

03c1873

fix bucket name and log bytes downloaded

9e8973b

Merge branch 'main' into feat-s3-example

8178acd

kggilmer merged commit 56857c4 into main May 7, 2021

kggilmer deleted the feat-s3-example branch May 7, 2021 23:28

		// Check for valid S3 configuration based on account
		suspend fun validateS3(s3Client: S3Client) {

Add s3 file ingestion sample to examples source tree for M1 release. #123

Add s3 file ingestion sample to examples source tree for M1 release. #123

Uh oh!

Conversation

kggilmer commented May 7, 2021

Testing done

Uh oh!

aajtodd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants