S3 relay interface #833

cody-littley · 2024-10-24T17:17:03Z

Why are these changes needed?

This PR adds utilities for uploading and downloading files from S3 in a way that supports breaking files into smaller ones.

Checks

I've made sure the lint is passing in this PR.
I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
I've checked the new test coverage and the coverage percentage didn't drop.
Testing Strategy
- Unit tests
- Integration tests
- This PR is not tested :(

Signed-off-by: Cody Littley <[email protected]>

dmanc

Looks great, some initial comments

dmanc · 2024-10-24T17:20:27Z

go.mod

@@ -6,6 +6,7 @@ toolchain go1.21.1

 require (
 	github.com/Layr-Labs/eigensdk-go v0.1.7-0.20240507215523-7e4891d5099a
+	github.com/aws/aws-sdk-go v1.55.5


Is it possible to do it with just v2 library?

Yes, refactored to only use v2 API.

dmanc · 2024-10-24T17:23:38Z

relay/dataplane/config.go

+	PrefixChars int
+	// This framework utilizes a pool of workers to help upload/download files. This value specifies the number of
+	// workers to use. Default is 32.
+	Parallelism int


Should we set the default as number of cpus in machine? Like by using gomaxprocs

I've turned this into two parameters: ParallelismFactor and ParallelismConstant. The total number of workers is set equal to the formula ParallelismFactor * numCores + ParallelismConstant.

We will want to have a lot more workers than cores, since most of the time the workers are blocked on IO tasks. This allows us to set a sane default that uses a good number of workers as the number of cores grows.

ian-shim

Looks good! Few comments

ian-shim · 2024-10-25T02:45:10Z

relay/dataplane/client.go

+// that are consumed by utilities that are not aware of the multipart upload/download process.
+//
+// Implementations of this interface are required to be thread-safe.
+type S3Client interface {


Why don't we implement new methods that support fragments in the existing s3 client (common/aws/s3)?

change made

ian-shim · 2024-10-25T02:51:36Z

relay/dataplane/fragment.go

+	if fragmentCount-1 == index {
+		postfix = "f"
+	}
+


should we also validate that index < fragmentCount ?

checks added

ian-shim · 2024-10-25T03:02:46Z

relay/dataplane/s3_client.go

+	// tasks are placed into this channel to be processed by workers.
+	tasks chan func()
+	// this wait group is completed when all workers have finished.
+	wg *sync.WaitGroup


I think what you're doing here can be simplified & abstracted out by using workerpool (ex. https://github.com/Layr-Labs/eigenda/blob/master/api/clients/retrieval_client.go#L181)

This is a neat little library, good to know about. Done.

Signed-off-by: Cody Littley <[email protected]>

ian-shim

lgtm!
Please resolve the test failures before merging

ian-shim · 2024-10-29T16:33:47Z

common/aws/cli.go

 )

 type ClientConfig struct {
-	Region          string
-	AccessKey       string
+	// The region to use when interacting with S3. Default is "us-east-2".


nit: start comment with the name of the field..

ian-shim · 2024-10-29T16:35:54Z

common/aws/cli.go

+	// This framework utilizes a pool of workers to help upload/download files. A non-zero value for this parameter
+	// adds a number of workers equal to the number of cores times this value. Default is 8. In general, the number
+	// of workers here can be a lot larger than the number of cores because the workers will be blocked on I/O most
+	// of the time.
+	FragmentParallelismFactor int
+	// This framework utilizes a pool of workers to help upload/download files. A non-zero value for this parameter
+	// adds a constant number of workers. Default is 0.
+	FragmentParallelismConstant int


Why not just use a single field to explicitly define the number of workers?

This is based on @dmanc's request to scale the number of workers based on the number of cores on the machine. The pattern here allows for the user to specify either a fixed number of threads or a number that varies with the number of cores.

If you think this is over complicated I'm willing to go back to having a constant number of worker threads.

ian-shim · 2024-10-29T16:40:42Z

common/aws/s3/client.go

+
+	for _, fragment := range fragments {
+		fragmentCapture := fragment
+		s.pool.Submit(func() {


This pool is shared across all ongoing uploads and downloads. Which means when there are many uploads/downloads in flight, it can build backpressure. Is that problematic?

My previous iteration had a config parameter that allowed the client to be have a configurable sized work queue (i.e. when I was using my own implementation of workerpool. Unfortunately, the workerpool library does not allow us to override the channel used to send data to the workers.

From the implementation:

workerQueue: make(chan func())

By default, channels have a buffer size of 0. This means that if the number of read/write tasks exceed the number of available workers, the caller will block until all tasks are accepted by a worker. This will provide back pressure if more work is scheduled than there are workers to handle that work.

Are you ok with the way this is configured by default? If not, we will probably need our own implementation of workerpool.

Signed-off-by: Cody Littley <[email protected]>

This reverts commit aca3040.

cody-littley added 7 commits October 23, 2024 09:12

Created core framework, need to add unit tests

6814f14

Signed-off-by: Cody Littley <[email protected]>

Add unit tests for fragmentation logic.

c7ba7d6

Signed-off-by: Cody Littley <[email protected]>

Incremental progress.

e5dcc78

Signed-off-by: Cody Littley <[email protected]>

Fix some bugs.

17ac4bc

Signed-off-by: Cody Littley <[email protected]>

Fixed bugs.

356959c

Signed-off-by: Cody Littley <[email protected]>

Test against localstack.

958e6e1

Signed-off-by: Cody Littley <[email protected]>

lint

a2bb06f

Signed-off-by: Cody Littley <[email protected]>

cody-littley requested review from dmanc, mooselumph, jianoaix and ian-shim October 24, 2024 17:17

cody-littley self-assigned this Oct 24, 2024

cody-littley added 2 commits October 24, 2024 12:17

Cleanup.

95b9d75

Signed-off-by: Cody Littley <[email protected]>

Merge branch 'master' into s3-relay-interface

9b9d57d

Signed-off-by: Cody Littley <[email protected]>

dmanc reviewed Oct 24, 2024

View reviewed changes

ian-shim reviewed Oct 25, 2024

View reviewed changes

cody-littley added 8 commits October 25, 2024 08:47

Remove TTL from upload

e6c7a0e

Signed-off-by: Cody Littley <[email protected]>

Merge branch 'master' into s3-relay-interface

b712c88

Signed-off-by: Cody Littley <[email protected]>

Use v2 APIs.

93e2310

Signed-off-by: Cody Littley <[email protected]>

Made suggested changes.

6e9aa2e

Signed-off-by: Cody Littley <[email protected]>

Made suggested changes.

8323596

Signed-off-by: Cody Littley <[email protected]>

Incremental checkin.

5d5f425

Signed-off-by: Cody Littley <[email protected]>

Finished migration.

2ddd2c4

Signed-off-by: Cody Littley <[email protected]>

Fix flags.

220d48f

Signed-off-by: Cody Littley <[email protected]>

cody-littley mentioned this pull request Oct 29, 2024

[v2] s3 Blob Store #841

Merged

6 tasks

cody-littley added 3 commits October 29, 2024 11:18

Fix bug.

4996fb0

Signed-off-by: Cody Littley <[email protected]>

Merge branch 'master' into s3-relay-interface

06cb92e

Fix unit test.

dc5b6fc

Signed-off-by: Cody Littley <[email protected]>

ian-shim approved these changes Oct 29, 2024

View reviewed changes

Fix unit test.

9104ab1

Signed-off-by: Cody Littley <[email protected]>

cody-littley added 8 commits October 29, 2024 11:58

Fix unit test.

319fca6

Signed-off-by: Cody Littley <[email protected]>

Fix unit test.

fe9e08c

Signed-off-by: Cody Littley <[email protected]>

Make suggested changes.

8f269de

Signed-off-by: Cody Littley <[email protected]>

Tweak unit test.

c5d5b96

Signed-off-by: Cody Littley <[email protected]>

Merge branch 'master' into s3-relay-interface

a4c081d

Signed-off-by: Cody Littley <[email protected]>

Change localstack port.

c4c7e2d

Signed-off-by: Cody Littley <[email protected]>

Add debug code.

684dab8

Signed-off-by: Cody Littley <[email protected]>

Fiddle with inabox settings.

958f593

Signed-off-by: Cody Littley <[email protected]>

cody-littley merged commit aca3040 into Layr-Labs:master Oct 30, 2024
5 of 6 checks passed

cody-littley deleted the s3-relay-interface branch October 30, 2024 17:11

cody-littley added a commit that referenced this pull request Oct 30, 2024

Revert "S3 relay interface (#833)"

56f5ae2

This reverts commit aca3040.

This was referenced Oct 30, 2024

Revert "S3 relay interface" #850

Merged

Revert "Revert "S3 relay interface"" #853

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

S3 relay interface #833

S3 relay interface #833

cody-littley commented Oct 24, 2024 •

edited

Loading

dmanc left a comment

dmanc Oct 24, 2024

cody-littley Oct 28, 2024

dmanc Oct 24, 2024

cody-littley Oct 28, 2024

ian-shim left a comment

ian-shim Oct 25, 2024

cody-littley Oct 28, 2024

ian-shim Oct 25, 2024

cody-littley Oct 28, 2024

ian-shim Oct 25, 2024

cody-littley Oct 28, 2024

ian-shim left a comment

ian-shim Oct 29, 2024

cody-littley Oct 29, 2024

ian-shim Oct 29, 2024

cody-littley Oct 29, 2024

ian-shim Oct 29, 2024

cody-littley Oct 29, 2024

S3 relay interface #833

S3 relay interface #833

Conversation

cody-littley commented Oct 24, 2024 • edited Loading

Why are these changes needed?

Checks

dmanc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ian-shim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ian-shim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cody-littley commented Oct 24, 2024 •

edited

Loading