From 9be01a7b1aa3b142259e05af4e2db49de57eae08 Mon Sep 17 00:00:00 2001 From: yahavi Date: Thu, 14 Sep 2023 15:35:24 +0300 Subject: [PATCH 1/4] Transfer - Send chunks if they are bigger than GiB --- .../commands/transferfiles/api/types.go | 19 +++++++ .../commands/transferfiles/api/types_test.go | 49 +++++++++++++++++++ .../commands/transferfiles/fulltransfer.go | 2 +- .../commands/transferfiles/transfer.go | 1 - artifactory/commands/transferfiles/utils.go | 2 +- 5 files changed, 70 insertions(+), 3 deletions(-) create mode 100644 artifactory/commands/transferfiles/api/types_test.go diff --git a/artifactory/commands/transferfiles/api/types.go b/artifactory/commands/transferfiles/api/types.go index b9f0d8768..cfdb5f057 100644 --- a/artifactory/commands/transferfiles/api/types.go +++ b/artifactory/commands/transferfiles/api/types.go @@ -23,6 +23,10 @@ const ( Phase1 int = 0 Phase2 int = 1 Phase3 int = 2 + + maxFilesInChunk = 16 + // 1GiB + maxBytesInChunk = 1 << (30) ) type TargetAuth struct { @@ -101,3 +105,18 @@ func (uc *UploadChunk) AppendUploadCandidateIfNeeded(file FileRepresentation, bu } uc.UploadCandidates = append(uc.UploadCandidates, file) } + +// Return true if the chunk contains at least 16 files or at least 1GiB in total +func (uc *UploadChunk) IsChunkFull() bool { + if len(uc.UploadCandidates) >= maxFilesInChunk { + return true + } + var totalSize int64 = 0 + for _, uploadCandidate := range uc.UploadCandidates { + totalSize += uploadCandidate.Size + if totalSize > maxBytesInChunk { + return true + } + } + return false +} diff --git a/artifactory/commands/transferfiles/api/types_test.go b/artifactory/commands/transferfiles/api/types_test.go new file mode 100644 index 000000000..a8b75c46f --- /dev/null +++ b/artifactory/commands/transferfiles/api/types_test.go @@ -0,0 +1,49 @@ +package api + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestAppendUploadCandidateIfNeeded(t *testing.T) { + uploadChunk := &UploadChunk{} + + // Regular file + uploadChunk.AppendUploadCandidateIfNeeded(FileRepresentation{Name: "regular-file"}, false) + assert.Len(t, uploadChunk.UploadCandidates, 1) + + // Build info + uploadChunk.AppendUploadCandidateIfNeeded(FileRepresentation{Name: "build-info.json"}, true) + assert.Len(t, uploadChunk.UploadCandidates, 2) + + // Directory in build info - should be skipped + uploadChunk.AppendUploadCandidateIfNeeded(FileRepresentation{}, true) + assert.Len(t, uploadChunk.UploadCandidates, 2) +} + +var isChunkFullCases = []struct { + files []FileRepresentation + isFull bool +}{ + {[]FileRepresentation{}, false}, + {[]FileRepresentation{{Name: "slim-jim", Size: 10737418}}, false}, + {[]FileRepresentation{{Name: "fat-vinny", Size: 1073741825}}, true}, +} + +func TestIsChunkFull(t *testing.T) { + for _, testCase := range isChunkFullCases { + t.Run("", func(t *testing.T) { + uploadChunk := &UploadChunk{UploadCandidates: testCase.files} + assert.Equal(t, testCase.isFull, uploadChunk.IsChunkFull()) + }) + } + t.Run("", func(t *testing.T) { + uploadChunk := &UploadChunk{} + for i := 0; i < 17; i++ { + uploadChunk.AppendUploadCandidateIfNeeded(FileRepresentation{Name: fmt.Sprintf("%d", i)}, false) + } + assert.True(t, uploadChunk.IsChunkFull()) + }) +} diff --git a/artifactory/commands/transferfiles/fulltransfer.go b/artifactory/commands/transferfiles/fulltransfer.go index 3229982d9..4e90d7564 100644 --- a/artifactory/commands/transferfiles/fulltransfer.go +++ b/artifactory/commands/transferfiles/fulltransfer.go @@ -239,7 +239,7 @@ func (m *fullTransferPhase) handleFoundFile(pcWrapper producerConsumerWrapper, return } curUploadChunk.AppendUploadCandidateIfNeeded(file, m.buildInfoRepo) - if len(curUploadChunk.UploadCandidates) == uploadChunkSize { + if curUploadChunk.IsChunkFull() { _, err = pcWrapper.chunkUploaderProducerConsumer.AddTaskWithError(uploadChunkWhenPossibleHandler(&m.phaseBase, *curUploadChunk, uploadChunkChan, errorsChannelMng), pcWrapper.errorsQueue.AddError) if err != nil { return diff --git a/artifactory/commands/transferfiles/transfer.go b/artifactory/commands/transferfiles/transfer.go index fb45a01dc..218a9fb28 100644 --- a/artifactory/commands/transferfiles/transfer.go +++ b/artifactory/commands/transferfiles/transfer.go @@ -28,7 +28,6 @@ import ( ) const ( - uploadChunkSize = 16 // Size of the channel where the transfer's go routines write the transfer errors fileWritersChannelSize = 500000 retries = 600 diff --git a/artifactory/commands/transferfiles/utils.go b/artifactory/commands/transferfiles/utils.go index 668bebc1d..31fcd5045 100644 --- a/artifactory/commands/transferfiles/utils.go +++ b/artifactory/commands/transferfiles/utils.go @@ -419,7 +419,7 @@ func uploadByChunks(files []api.FileRepresentation, uploadTokensChan chan Upload continue } curUploadChunk.AppendUploadCandidateIfNeeded(file, base.buildInfoRepo) - if len(curUploadChunk.UploadCandidates) == uploadChunkSize { + if curUploadChunk.IsChunkFull() { _, err = pcWrapper.chunkUploaderProducerConsumer.AddTaskWithError(uploadChunkWhenPossibleHandler(&base, curUploadChunk, uploadTokensChan, errorsChannelMng), pcWrapper.errorsQueue.AddError) if err != nil { return From d4416dd511403d65c813e3f67fb83760879ef04e Mon Sep 17 00:00:00 2001 From: yahavi Date: Wed, 20 Sep 2023 09:21:35 +0300 Subject: [PATCH 2/4] CR --- .../commands/transferfiles/api/types_test.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/artifactory/commands/transferfiles/api/types_test.go b/artifactory/commands/transferfiles/api/types_test.go index a8b75c46f..f048ed525 100644 --- a/artifactory/commands/transferfiles/api/types_test.go +++ b/artifactory/commands/transferfiles/api/types_test.go @@ -39,11 +39,12 @@ func TestIsChunkFull(t *testing.T) { assert.Equal(t, testCase.isFull, uploadChunk.IsChunkFull()) }) } - t.Run("", func(t *testing.T) { - uploadChunk := &UploadChunk{} - for i := 0; i < 17; i++ { - uploadChunk.AppendUploadCandidateIfNeeded(FileRepresentation{Name: fmt.Sprintf("%d", i)}, false) - } - assert.True(t, uploadChunk.IsChunkFull()) - }) +} + +func TestIsChunkFullNumberOfFiles(t *testing.T) { + uploadChunk := &UploadChunk{} + for i := 0; i < maxFilesInChunk+1; i++ { + uploadChunk.AppendUploadCandidateIfNeeded(FileRepresentation{Name: fmt.Sprintf("%d", i)}, false) + } + assert.True(t, uploadChunk.IsChunkFull()) } From 626b0b00eb7d861010931b734a37ae8ee56900be Mon Sep 17 00:00:00 2001 From: yahavi Date: Wed, 20 Sep 2023 09:22:34 +0300 Subject: [PATCH 3/4] Remove redundant parenthesis --- artifactory/commands/transferfiles/api/types.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/artifactory/commands/transferfiles/api/types.go b/artifactory/commands/transferfiles/api/types.go index cfdb5f057..9cab62860 100644 --- a/artifactory/commands/transferfiles/api/types.go +++ b/artifactory/commands/transferfiles/api/types.go @@ -25,8 +25,8 @@ const ( Phase3 int = 2 maxFilesInChunk = 16 - // 1GiB - maxBytesInChunk = 1 << (30) + // 1 GiB + maxBytesInChunk = 1 << 30 ) type TargetAuth struct { From 224e55d67c1918ff4a586712ad7d6db0196c6a1a Mon Sep 17 00:00:00 2001 From: yahavi Date: Wed, 20 Sep 2023 09:26:01 +0300 Subject: [PATCH 4/4] Improve test --- artifactory/commands/transferfiles/api/types_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/artifactory/commands/transferfiles/api/types_test.go b/artifactory/commands/transferfiles/api/types_test.go index f048ed525..c2ef3b207 100644 --- a/artifactory/commands/transferfiles/api/types_test.go +++ b/artifactory/commands/transferfiles/api/types_test.go @@ -43,7 +43,8 @@ func TestIsChunkFull(t *testing.T) { func TestIsChunkFullNumberOfFiles(t *testing.T) { uploadChunk := &UploadChunk{} - for i := 0; i < maxFilesInChunk+1; i++ { + for i := 0; i < maxFilesInChunk; i++ { + assert.False(t, uploadChunk.IsChunkFull()) uploadChunk.AppendUploadCandidateIfNeeded(FileRepresentation{Name: fmt.Sprintf("%d", i)}, false) } assert.True(t, uploadChunk.IsChunkFull())