feedback #3
feedback #3
Conversation
There was a problem hiding this comment.
rename the file to have the same nomenclature that we have in the sdk public repos.
|
|
||
| ## Features | ||
|
|
||
| - Batch upload multiple objects into a single S3 file, reducing the number of PUT operations. | ||
| - Retrieve individual objects using index information (byte offset and length). | ||
|
|
||
| // should we add a section explaining the advantajes of using this module? i mean reduce costs doing put operations, improve |
There was a problem hiding this comment.
I prefer to leave that to the document (blog post) and not here. But I guess we could add some quick explanation. Feel free to add anything you think it's worth!
There was a problem hiding this comment.
@pablomatiasgomez i agree with @santileira. if people discover this repo in the future they may not read the blog post. i would suggest adding something like to the first part of the README.md
This method of storage and retrieval is well suited for write-heavy workloads, where you want to fetch a small percentage of the stored objects later. The storage approach also works well when you have objects of widely varying size.
There was a problem hiding this comment.
hmm right! I will open a separate PR with more details in the README !
There was a problem hiding this comment.
Added a better description in #11 let me know your thoughts!
| @@ -12,10 +12,11 @@ import ( | |||
| // To create a new file, first call NewTempFile, then append objects to it, and finally call UploadToS3. | |||
| // After the file is uploaded, you can save the object indexes to a database, and use them to fetch the objects later. | |||
| // To fetch the contents of a single object, call Fetch with the ObjectIndex that you had stored. | |||
| // is this space needed? | |||
There was a problem hiding this comment.
I usually leave a space between the godoc and go gen 🤔 but it's the same for me. Feel free to remove it if you like it more that way
| @@ -9,6 +9,8 @@ import ( | |||
| "github.com/aws/aws-sdk-go-v2/service/s3" | |||
| ) | |||
|
|
|||
| // i will put the Fetch, NewTempFile, UploadToS3, DeleteFromS3 methods on the same file because they are part of the | |||
| // same interface. | |||
There was a problem hiding this comment.
hmm I don't agree, it would end up being a huge single file. What would be the benefit of doing that?
There was a problem hiding this comment.
My point is that we are going to have the interface in one file and then the implementation in different files so it is hard to see the whole implementation at once but...
| @@ -100,16 +101,19 @@ func (f *TempFile[K]) Tags() map[string]string { | |||
| } | |||
|
|
|||
| // Age returns the duration since this file has been started | |||
| // do we need this method? we are just using it in tests, maybe we can remove it and remove the created on attribute. | |||
There was a problem hiding this comment.
We need to use this in our loader, to know when a file is so old that we have to upload it. Basically, age, count, and size provide information about this file so that the caller can make decisions and do different things based on them
There was a problem hiding this comment.
ah, makes sense. interesting that we need to add things here that we are not "actually" using on the module... anyone reading this will have the same question than me.
There was a problem hiding this comment.
I mean, in modules there's always something exposed that is not explicitly used. The same would apply with NewClient 😅 . I think it's reasonable to have these cases
There was a problem hiding this comment.
ah, makes sense. interesting that we need to add things here that we are not "actually" using on the module... anyone reading this will have the same question than me.
When I see this in a public repo it helps to have an example of how they expect it to be used
There was a problem hiding this comment.
Good point, the example covers most of the cases but not these methods. I will update it.
No description provided.