Revert "Revert "S3 relay interface"" #853
Conversation
This reverts commit 855096a.
Signed-off-by: Cody Littley <cody@eigenlabs.org>
Signed-off-by: Cody Littley <cody@eigenlabs.org>
Signed-off-by: Cody Littley <cody@eigenlabs.org>
Signed-off-by: Cody Littley <cody@eigenlabs.org>
Signed-off-by: Cody Littley <cody@eigenlabs.org>
Signed-off-by: Cody Littley <cody@eigenlabs.org>
Signed-off-by: Cody Littley <cody@eigenlabs.org>
| FragmentPrefixChars int | ||
| // FragmentParallelismFactor helps determine the size of the 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 | ||
| // FragmentParallelismConstant helps determine the size of the 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 | ||
| // FragmentReadTimeout is used to bound the maximum time to wait for a single fragmented read. | ||
| // Default is 30 seconds. | ||
| FragmentReadTimeout time.Duration | ||
| // FragmentWriteTimeout is used to bound the maximum time to wait for a single fragmented write. | ||
| // Default is 30 seconds. | ||
| FragmentWriteTimeout time.Duration |
There was a problem hiding this comment.
These seem like S3 specific configs, but they are in the common shared aws config. Should we consider keeping this one only for parameters that are shared between all the aws clients we use (s3, dynamo, etc)?
So perhaps create a config specific in aws/s3/config.go which would have these fragments and would embed the shared aws.ClientConfig params?
There was a problem hiding this comment.
Your suggestion makes sense. The core difficulty is that aws.ClientConfig is currently used to build an S3 client by disperser/cmd/apiserver, disperser/cmd/batcher, and disperser/cmd/dataapi. Even harder is the fact that these use cases use the aws.ClientConfig object to instantiate both an s3 client and a dynamoDB client, meaning that if I nest aws.ClientConfig inside S3 config then it gets duplicated.
I'm sure it's possible to untangle the mess I describe above, but my concern is scope creep for this PR. I've got some ideas for how we might simplify the way we manage our project's configuration, would you be interested in discussing this offline?
There was a problem hiding this comment.
Yes, interested in discussing, no need for scope creep I agree, since these are only used by us internally so its fine to have breaking changes.
That being said, I don't see why having the duplicate ClientConfig is bad. In fact, one could want an s3 client and dynamoDB client with different aws configs (say the dbs live in different regions for eg).
| // Example: fileKey="abc123", prefixLength=2, fragmentCount=3 | ||
| // The keys will be "ab/abc123-0", "ab/abc123-1", "ab/abc123-2f" |
There was a problem hiding this comment.
S3 throttles bandwidth based on prefix. That means reading and writing keys with two different prefixes has an aggregate upper bandwidth limit twice as high as reading and writing keys with a single prefix.
https://docs.aws.amazon.com/AmazonS3/latest/userguide/optimizing-performance.html
For example, your application can achieve at least 3,500 PUT/COPY/POST/DELETE or 5,500 GET/HEAD requests per second per partitioned Amazon S3 prefix. There are no limits to the number of prefixes in a bucket. You can increase your read or write performance by using parallelization. For example, if you create 10 prefixes in an Amazon S3 bucket to parallelize reads, you could scale your read performance to 55,000 read requests per second. Similarly, you can scale write operations by writing to multiple prefixes.
The goal here is to spread the data into different prefixes so that we have a much higher upper limit on bandwidth.
There was a problem hiding this comment.
This is cool, didn't know about it. But I'm still a bit puzzled by how you are making use of this though. Given that the prefix length is a global config that is always the same, then all fragments of a file will have the same prefix right? Compare
"ab/abc123-0", "ab/abc123-1", "ab/abc123-2f"
to
"abc123-0", "abc123-1", "abc123-2f"
All fragments in both cases have the same prefix, so I don't quite understand how using an extra prefix is helping here. Am I missing something?
There was a problem hiding this comment.
+1
The prefix helps with creating virtual hierarchy within the bucket, but all files with the same prefix is already in the same bucket, right?
There was a problem hiding this comment.
All files end up in the same bucket. The prefix has nothing to do with which bucket each file is written to. Our use of prefixes is entirely a tactic to improve our bandwidth with S3, not as a mechanism used to organize our data for our own purposes.
There was a problem hiding this comment.
I see. Ok so the idea is that all files are in the same bucket, which means without prefixes all reads/writes to this bucket would have a total bandwidth limit. Since we want a per-file bandwidth limit, you add these prefixes.
Why don't you just use "-" as the prefix delimiter then, instead of adding this prefix? Still feels to me like the real useful prefix is the file name, not this deterministically shortened version of it (which I still think isn't useful), no?
On a side note, how exactly are buckets organized? When do we create a new bucket, if ever?
common/aws/test/fragment_test.go
Outdated
| } | ||
| } | ||
|
|
||
| func TestExampleInGodoc(t *testing.T) { |
There was a problem hiding this comment.
Is the goal for this function to appear in godoc? I think for that to happen the function name has to start with Example and not Test: https://go.dev/blog/examples
There was a problem hiding this comment.
I give an example in the godoc of the function getFragmentKey(). Updated the godoc to mention that:
// TestExampleInGodoc tests the example provided in the documentation for getFragmentKey().
//
// Example: fileKey="abc123", prefixLength=2, fragmentCount=3
// The keys will be "ab/abc123-0", "ab/abc123-1", "ab/abc123-2f"
There was a problem hiding this comment.
Gotcha. I was suggesting renaming this function to ExampleGetFragmentKey() so that the example would also appear in the godoc (see https://go.dev/blog/examples), but now that we've made those functions private I guess there is no point.
Signed-off-by: Cody Littley <cody@eigenlabs.org>
Signed-off-by: Cody Littley <cody@eigenlabs.org>
| // Example: fileKey="abc123", prefixLength=2, fragmentCount=3 | ||
| // The keys will be "ab/abc123-0", "ab/abc123-1", "ab/abc123-2f" |
There was a problem hiding this comment.
+1
The prefix helps with creating virtual hierarchy within the bucket, but all files with the same prefix is already in the same bucket, right?
Signed-off-by: Cody Littley <cody@eigenlabs.org>
Reverts #850
I merged this PR which caused a CI breakage. I reverted the change to unblock others. This PR re-enables the changes with the required fixes in place.
There are two lines that deviate from the original PR:
inabox/deploy/localstack.go. This was not the cause of the failure, but an intentional oversight that created some spam in stout.common/cli.gothat was misconfiguration. This was the root cause of the failing integration-tests panel.