[Buffered Writes] bucket interface changes for buffered writes part 1 #2597

ashmeenkaur · 2024-10-15T10:51:35Z

Description

This PR includes changes to bucket.go interface and all it's associated implementations to include the following methods:

CreateObjectChunkWriter()
FinalizeUpload()
fake bucket, prefix bucket and and content type bucket implementations are added in a subsequent PR [Buffered Writes] bucket interface changes for buffered writes part 2 #2606

Link to the issue in case of a bug fix.

NA

Testing details

Manual - NA
Unit tests - Added
Integration tests - NA

codecov · 2024-10-18T04:13:38Z

Codecov Report

Attention: Patch coverage is 54.72973% with 67 lines in your changes missing coverage. Please review.

Project coverage is 78.28%. Comparing base (0dab668) to head (f111264).
Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/ratelimit/throttled_bucket.go	0.00%	15 Missing ⚠️
internal/storage/debug_bucket.go	0.00%	12 Missing ⚠️
internal/monitor/bucket.go	0.00%	10 Missing ⚠️
internal/storage/testify_mock_bucket.go	0.00%	9 Missing ⚠️
internal/gcsx/prefix_bucket.go	0.00%	6 Missing ⚠️
internal/storage/fake/bucket.go	0.00%	6 Missing ⚠️
internal/storage/mock_bucket.go	90.47%	2 Missing and 2 partials ⚠️
internal/storage/storageutil/object_attrs.go	0.00%	2 Missing and 1 partial ⚠️
internal/storage/bucket_handle.go	92.59%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2597      +/-   ##
==========================================
- Coverage   78.48%   78.28%   -0.21%     
==========================================
  Files         108      109       +1     
  Lines       15905    16056     +151     
==========================================
+ Hits        12483    12569      +86     
- Misses       2914     2976      +62     
- Partials      508      511       +3

Flag	Coverage Δ
unittests	`78.28% <54.72%> (-0.21%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vadlakondaswetha · 2024-10-18T10:41:52Z

internal/ratelimit/throttled_bucket.go

+
+func (b *throttledBucket) FinalizeUpload(ctx context.Context, w gcs.Writer) (*gcs.Object, error) {
+	// Wait for permission to call through.
+	err := b.opThrottle.Wait(ctx, 1)


I think we shouldn't throttle this operation given that this fails, entire upload fails. Thoughts?

Yes, I think it is okay to just throttle for create writer operation as they will anyway share the context. We are in a way double counting here too. Thanks for catching this!

vadlakondaswetha · 2024-10-18T10:46:07Z

internal/storage/gcs/bucket.go

@@ -43,6 +44,12 @@ const (
 	ReqIdField string = "GcsReqId"
 )

+type Writer interface {


Can you add a comment here why is this interface required and why can't use storage.writer directly. Atleast i didnt understand the usecase.

We need different implementations of writer for testing - particularly for fake bucket that we use for composite tests. That is why I added this interface. Fake bucket writer implementation is in the subsequent PR.

vadlakondaswetha · 2024-10-18T10:46:54Z

internal/storage/storageutil/object_attrs.go

@@ -62,6 +62,9 @@ func convertACLRuleToObjectAccessControl(element storage.ACLRule) *storagev1.Obj
 }

 func ObjectAttrsToBucketObject(attrs *storage.ObjectAttrs) *gcs.Object {
+	if attrs == nil {


Did it happen in any scenario without receiving an error from GCS. Just curious

No not a real scenario, this is added just to make unit tests simpler.

vadlakondaswetha · 2024-10-18T13:43:53Z

internal/storage/bucket_handle.go

+func (bh *bucketHandle) CreateObjectChunkWriter(ctx context.Context, req *gcs.CreateObjectRequest, chunkSize int, callBack func(bytesUploadedSoFar int64)) (gcs.Writer, error) {
+	// For phase 1 of buffered writes, we are doing chunk uploads only for new
+	// file uploads.
+	preconditions := storage.Conditions{}


why do you want to restrict for not exist scenario. why can't we just use the generation from the request

I was restricting to phase 1 of the project but makes sense to do this in one go as well. I'll update this.

internal/storage/bucket_handle_test.go

vadlakondaswetha · 2024-10-18T13:48:33Z

internal/storage/bucket_handle_test.go

+}
+
+func (testSuite *BucketHandleTest) createObject(objName string) {
+	testSuite.T().Helper()


I see this as definition for helper:
Helper marks the calling function as a test helper function.
// When printing file and line information, that function will be skipped.
// Helper may be called simultaneously from multiple goroutines.

Don't we want to print file and line info when something goes wrong?

This will still print the error but not with the file name and line number of test helper but with the file and line number of calling Test function so it is easier to debug from where the error is coming from.

Eg: With Test Helper:

=== RUN TestBucketHandleTestSuite/TestFinalizeUploadWithGenerationAsZeroWhenObjectAlreadyExists bucket_handle_test.go:554: Error Trace: /usr/local/google/home/ashmeen/gcsfuse/internal/storage/bucket_handle_test.go:543 /usr/local/google/home/ashmeen/gcsfuse/internal/storage/bucket_handle_test.go:554 Error: An error is expected but got nil. Test: TestBucketHandleTestSuite/TestFinalizeUploadWithGenerationAsZeroWhenObjectAlreadyExists

bucket_handle_test.go:554: is the line number of test function where helper is called

Without test helper:

=== RUN TestBucketHandleTestSuite/TestFinalizeUploadWithGenerationAsZeroWhenObjectAlreadyExists bucket_handle_test.go:543: Error Trace: /usr/local/google/home/ashmeen/gcsfuse/internal/storage/bucket_handle_test.go:543 /usr/local/google/home/ashmeen/gcsfuse/internal/storage/bucket_handle_test.go:554 Error: An error is expected but got nil. Test: TestBucketHandleTestSuite/TestFinalizeUploadWithGenerationAsZeroWhenObjectAlreadyExists --- FAIL: TestBucketHandleTestSuite (0.01s)

bucket_handle_test.go:543: is the line number of helper function where test is called.

internal/storage/caching/fast_stat_bucket_test.go

vadlakondaswetha · 2024-10-18T13:52:18Z

internal/storage/caching/fast_stat_bucket_test.go

+
+func init() { RegisterTestSuite(&CreateObjectChunkWriterTest{}) }
+
+func (t *CreateObjectChunkWriterTest) CallsWrapped() {


are you verifying the request parameters here. can you name the method accordingly

This test is validating that the wrapped bucket is called with the expected parameters. Let me know if you still think we should rename the test.

ashmeenkaur changed the title ~~[Do not review yet] bucket interface changes to storage.Writer for resumable uploads~~ [Do not review yet] bucket interface changes to create storage.Writer for resumable uploads Oct 15, 2024

ashmeenkaur force-pushed the resumable_upload_chunks branch from ebaa65a to 22aebfd Compare October 18, 2024 04:03

ashmeenkaur marked this pull request as ready for review October 18, 2024 05:18

ashmeenkaur requested a review from a team as a code owner October 18, 2024 05:18

ashmeenkaur requested a review from kislaykishore October 18, 2024 05:18

ashmeenkaur changed the title ~~[Do not review yet] bucket interface changes to create storage.Writer for resumable uploads~~ [Buffered Writes] bucket interface changes for buffered writes Oct 18, 2024

kislaykishore requested a review from a team October 18, 2024 05:18

ashmeenkaur requested review from vadlakondaswetha and removed request for a team October 18, 2024 05:18

kislaykishore requested review from a team and removed request for vadlakondaswetha October 18, 2024 05:18

ashmeenkaur requested review from vadlakondaswetha and removed request for a team October 18, 2024 05:20

ashmeenkaur added the execute-integration-tests Run only integration tests label Oct 18, 2024

kislaykishore requested a review from a team October 18, 2024 05:20

ashmeenkaur force-pushed the resumable_upload_chunks branch from 38812e9 to fd305af Compare October 18, 2024 06:07

ashmeenkaur added minimal-file-buffering and removed execute-integration-tests Run only integration tests labels Oct 18, 2024

ashmeenkaur added 10 commits October 18, 2024 07:21

bucket interface changes to storage.Writer for resumable uploads

dbb4c0d

bucket interface changes to finalize upload for resumable upload

9e349ed

add unit tests

c0aeb0b

added fake bucket implementation

f5adf8a

test fixes

f68aaf0

lint fixes

791337a

header fixes

49eeb24

unit tests

3e6af6f

unit tests

9873760

split PR

82acee7

ashmeenkaur force-pushed the resumable_upload_chunks branch from fd305af to 82acee7 Compare October 18, 2024 07:24

revert changes

c8bc441

ashmeenkaur changed the title ~~[Buffered Writes] bucket interface changes for buffered writes~~ [Buffered Writes] bucket interface changes for buffered writes part 1 Oct 18, 2024

revert changes to prefix bucket

1096bd3

ashmeenkaur mentioned this pull request Oct 18, 2024

[Buffered Writes] bucket interface changes for buffered writes part 2 #2606

Open

vadlakondaswetha reviewed Oct 18, 2024

View reviewed changes

review comments

999adf3

ashmeenkaur requested a review from vadlakondaswetha October 22, 2024 08:38

review comment

f111264

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Buffered Writes] bucket interface changes for buffered writes part 1 #2597

[Buffered Writes] bucket interface changes for buffered writes part 1 #2597

ashmeenkaur commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 18, 2024 •

edited

Loading

vadlakondaswetha Oct 18, 2024

ashmeenkaur Oct 22, 2024

vadlakondaswetha Oct 18, 2024

ashmeenkaur Oct 22, 2024

vadlakondaswetha Oct 18, 2024

ashmeenkaur Oct 22, 2024

vadlakondaswetha Oct 18, 2024

ashmeenkaur Oct 22, 2024

vadlakondaswetha Oct 18, 2024

ashmeenkaur Oct 22, 2024 •

edited

Loading

vadlakondaswetha Oct 18, 2024

ashmeenkaur Oct 22, 2024


		func init() { RegisterTestSuite(&CreateObjectChunkWriterTest{}) }

		func (t *CreateObjectChunkWriterTest) CallsWrapped() {

[Buffered Writes] bucket interface changes for buffered writes part 1 #2597

Are you sure you want to change the base?

[Buffered Writes] bucket interface changes for buffered writes part 1 #2597

Conversation

ashmeenkaur commented Oct 15, 2024 • edited Loading

Description

Link to the issue in case of a bug fix.

Testing details

codecov bot commented Oct 18, 2024 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashmeenkaur Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashmeenkaur commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 18, 2024 •

edited

Loading

ashmeenkaur Oct 22, 2024 •

edited

Loading