[patch] Revise S3 reader/writer: compatible with IBM Cloud Object Storage #509

rinx · 2020-06-23T10:11:28Z

Signed-off-by: Rintaro Okamura rintaro.okamura@gmail.com

Description:

Add max_chunk_size option
Add backoff option to S3 reader
Add internal/io package

Related Issue:

Nothing.

How Has This Been Tested?:

On GKE.

Environment:

Go Version: 1.14.3
Docker Version: 19.03.8
Kubernetes Version: 1.18.2
NGT Version: 1.11.5

Types of changes:

Bug fix [type/bug]
New feature [type/feature]
Add tests [type/test]
Security related changes [type/security]
Add documents [type/documentation]
Refactoring [type/refactoring]
Update dependencies [type/dependency]
Update benchmarks and performances [type/bench]
Update CI [type/ci]

Changes to Core Features:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your core changes, as applicable?
Have you successfully ran tests with your changes locally?

Checklist:

I have read the CONTRIBUTING document.
I have checked open Pull Requests for the similar feature or fixes?
I have added tests and benchmarks to cover my changes.
I have ensured all new and existing tests passed.
I have commented my code, particularly in hard-to-understand areas
I have updated the documentation accordingly.

pull-assistant · 2020-06-23T10:11:30Z

Best reviewed: commit by commit

Optimal code review plan (1 warning)

🔧 revise downloading s3 parts

✨ Add internal/io package

✨ Add backoff to s3 reader

...orage/blob/s3/reader/reader.go 70% changes removed in 🔧 add bytes.B...

🔧 add bytes.Buffer to s3 reader

♻️ use defer for Close()

Powered by Pull Assistant. Last update 5b3c39d ... e8f125f. Read the comment docs.

codecov · 2020-06-23T10:20:24Z

Codecov Report

Merging #509 into master will decrease coverage by 0.03%.
The diff coverage is 2.45%.

@@            Coverage Diff            @@
##           master    #509      +/-   ##
=========================================
- Coverage    8.31%   8.28%   -0.04%     
=========================================
  Files         401     402       +1     
  Lines       20710   20836     +126     
=========================================
+ Hits         1722    1726       +4     
- Misses      18742   18860     +118     
- Partials      246     250       +4

Impacted Files	Coverage Δ
internal/config/blob.go	`0.00% <0.00%> (ø)`
internal/config/sidecar.go	`0.00% <ø> (ø)`
internal/db/storage/blob/s3/option.go	`2.50% <0.00%> (-2.05%)`	⬇️
internal/db/storage/blob/s3/reader/reader.go	`0.00% <0.00%> (ø)`
internal/db/storage/blob/s3/s3.go	`0.00% <0.00%> (ø)`
internal/db/storage/blob/s3/writer/writer.go	`0.00% <0.00%> (ø)`
internal/errors/io.go	`0.00% <0.00%> (ø)`
pkg/agent/sidecar/service/observer/observer.go	`0.00% <0.00%> (ø)`
pkg/agent/sidecar/service/restorer/restorer.go	`0.00% <0.00%> (ø)`
...ent/sidecar/usecase/initcontainer/initcontainer.go	`0.00% <0.00%> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61fed82...e8f125f. Read the comment docs.

vdaas-ci · 2020-06-24T01:41:22Z

[WARNING:INTCFG] Changes in interal/config may require you to change Helm charts. Please check.

pkg/agent/sidecar/service/restorer/restorer.go

internal/config/blob.go

internal/db/storage/blob/s3/reader/reader.go

github-actions · 2020-06-25T03:26:42Z

internal/db/storage/blob/s3/reader/reader.go

+	r.wg.Add(1)
+	r.eg.Go(safety.RecoverFunc(func() (err error) {
+		defer r.wg.Done()
+		defer pw.Close()


[golangci] _{reported by reviewdog 🐶}
Error return value of pw.Close is not checked (errcheck)

pkg/agent/sidecar/service/restorer/option.go

Signed-off-by: Rintaro Okamura <rintaro.okamura@gmail.com>

internal/db/storage/blob/s3/reader/option.go

internal/db/storage/blob/s3/reader/reader.go

Signed-off-by: Rintaro Okamura <rintaro.okamura@gmail.com>

kpango

LGTM

github-actions · 2020-06-25T09:57:06Z

internal/db/storage/blob/s3/reader/option.go

+	}
+}
+
+func WithBackoff(enabled bool) Option {


[golangci] _{reported by reviewdog 🐶}
exported function WithBackoff should have comment or be unexported (golint)

github-actions · 2020-06-25T09:57:06Z

internal/db/storage/blob/s3/reader/option.go

+	}
+}
+
+func WithBackoffOpts(opts ...backoff.Option) Option {


[golangci] _{reported by reviewdog 🐶}
exported function WithBackoffOpts should have comment or be unexported (golint)

github-actions · 2020-06-25T09:57:06Z

internal/db/storage/blob/s3/reader/reader.go

+			body, err := func() (io.Reader, error) {
+				if r.backoffEnabled {
+					return r.getObjectWithBackoff(ctx, offset, r.maxChunkSize)
+				} else {


[golangci] _{reported by reviewdog 🐶}
if block ends with a return statement, so drop this else and outdent its block (golint)

github-actions · 2020-06-25T09:57:06Z

internal/db/storage/blob/s3/reader/reader.go

+}
+
+func (r *reader) getObject(ctx context.Context, offset, length int64) (io.Reader, error) {
+	log.Debugf("reading %d-%d bytes...", offset, offset+length-1)


[golangci] _{reported by reviewdog 🐶}
mnd: Magic number: 1, in detected (gomnd)

github-actions · 2020-06-25T09:57:06Z

internal/errors/io.go

+
+var (
+	// io
+	NewErrContextNotProvided = func() error {


[golangci] _{reported by reviewdog 🐶}
NewErrContextNotProvided is a global variable (gochecknoglobals)

github-actions · 2020-06-25T09:57:07Z

internal/errors/io.go

+		return New("context not provided")
+	}
+
+	NewErrReaderNotProvided = func() error {


[golangci] _{reported by reviewdog 🐶}
NewErrReaderNotProvided is a global variable (gochecknoglobals)

github-actions · 2020-06-25T09:57:07Z

internal/errors/io.go

+		return New("io.Reader not provided")
+	}
+
+	NewErrWriterNotProvided = func() error {


[golangci] _{reported by reviewdog 🐶}
NewErrWriterNotProvided is a global variable (gochecknoglobals)

kpango · 2020-06-25T10:02:47Z

/changelog

vdaas-ci · 2020-06-25T10:03:11Z

[CHANGELOG] Please edit the following lines.

Revise S3 reader/writer: compatible with IBM Cloud Object Storage (#509)
Fix roundtrip error handling (#508)
Update Helm Chart info (#496)

rinx added the status/draft label Jun 23, 2020

github-actions bot added area/internal size/S type/feature New feature priority/medium labels Jun 23, 2020

github-actions bot added area/agent/sidecar area/helm size/M and removed size/S labels Jun 24, 2020

rinx force-pushed the feature/internal-storage-s3/add-writer-params branch 2 times, most recently from bfe78a0 to e37810d Compare June 24, 2020 02:34

github-actions bot reviewed Jun 24, 2020

View reviewed changes

pkg/agent/sidecar/service/restorer/restorer.go Show resolved Hide resolved

rinx force-pushed the feature/internal-storage-s3/add-writer-params branch 2 times, most recently from da242bf to 0bb0d0c Compare June 25, 2020 03:16

github-actions bot added size/L and removed size/M labels Jun 25, 2020

rinx force-pushed the feature/internal-storage-s3/add-writer-params branch from 0bb0d0c to 29a0740 Compare June 25, 2020 03:18

rinx changed the title ~~Add request params to S3 writer~~ Revise S3 reader/writer: compatible with IBM Cloud Object Storage Jun 25, 2020

github-actions bot reviewed Jun 25, 2020

View reviewed changes

rinx force-pushed the feature/internal-storage-s3/add-writer-params branch from 29a0740 to de0c46b Compare June 25, 2020 03:30

github-actions bot reviewed Jun 25, 2020

View reviewed changes

pkg/agent/sidecar/service/restorer/option.go Show resolved Hide resolved

🔧 revise downloading s3 parts

5b3c39d

Signed-off-by: Rintaro Okamura <rintaro.okamura@gmail.com>

rinx force-pushed the feature/internal-storage-s3/add-writer-params branch from de0c46b to 5b3c39d Compare June 25, 2020 06:02

github-actions bot reviewed Jun 25, 2020

View reviewed changes

internal/db/storage/blob/s3/reader/option.go Show resolved Hide resolved

internal/db/storage/blob/s3/reader/option.go Show resolved Hide resolved

internal/db/storage/blob/s3/reader/reader.go Show resolved Hide resolved

internal/db/storage/blob/s3/reader/reader.go Show resolved Hide resolved

✨ Add internal/io package

18ac866

Signed-off-by: Rintaro Okamura <rintaro.okamura@gmail.com>

rinx force-pushed the feature/internal-storage-s3/add-writer-params branch from 59faa29 to 18ac866 Compare June 25, 2020 07:44

✨ Add backoff to s3 reader

e5f4182

Signed-off-by: Rintaro Okamura <rintaro.okamura@gmail.com>

🔧 add bytes.Buffer to s3 reader

e13c15d

Signed-off-by: Rintaro Okamura <rintaro.okamura@gmail.com>

rinx force-pushed the feature/internal-storage-s3/add-writer-params branch from 5d6b674 to e13c15d Compare June 25, 2020 09:25

rinx marked this pull request as ready for review June 25, 2020 09:44

♻️ use defer for Close()

e8f125f

Signed-off-by: Rintaro Okamura <rintaro.okamura@gmail.com>

kpango approved these changes Jun 25, 2020

View reviewed changes

github-actions bot reviewed Jun 25, 2020

View reviewed changes

kpango changed the title ~~Revise S3 reader/writer: compatible with IBM Cloud Object Storage~~ [patch] Revise S3 reader/writer: compatible with IBM Cloud Object Storage Jun 25, 2020

rinx merged commit c6fcb37 into master Jun 25, 2020

rinx deleted the feature/internal-storage-s3/add-writer-params branch June 25, 2020 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[patch] Revise S3 reader/writer: compatible with IBM Cloud Object Storage #509

[patch] Revise S3 reader/writer: compatible with IBM Cloud Object Storage #509

rinx commented Jun 23, 2020 •

edited

Loading

pull-assistant bot commented Jun 23, 2020 •

edited

Loading

codecov bot commented Jun 23, 2020 •

edited

Loading

vdaas-ci commented Jun 24, 2020

github-actions bot Jun 25, 2020

kpango left a comment

github-actions bot Jun 25, 2020

github-actions bot Jun 25, 2020

github-actions bot Jun 25, 2020

github-actions bot Jun 25, 2020

github-actions bot Jun 25, 2020

github-actions bot Jun 25, 2020

github-actions bot Jun 25, 2020

kpango commented Jun 25, 2020

vdaas-ci commented Jun 25, 2020 •

edited by rinx

Loading

[patch] Revise S3 reader/writer: compatible with IBM Cloud Object Storage #509

[patch] Revise S3 reader/writer: compatible with IBM Cloud Object Storage #509

Conversation

rinx commented Jun 23, 2020 • edited Loading

Description:

Related Issue:

How Has This Been Tested?:

Environment:

Types of changes:

Changes to Core Features:

Checklist:

pull-assistant bot commented Jun 23, 2020 • edited Loading

codecov bot commented Jun 23, 2020 • edited Loading

Codecov Report

vdaas-ci commented Jun 24, 2020

github-actions bot Jun 25, 2020

Choose a reason for hiding this comment

kpango left a comment

Choose a reason for hiding this comment

github-actions bot Jun 25, 2020

Choose a reason for hiding this comment

github-actions bot Jun 25, 2020

Choose a reason for hiding this comment

github-actions bot Jun 25, 2020

Choose a reason for hiding this comment

github-actions bot Jun 25, 2020

Choose a reason for hiding this comment

github-actions bot Jun 25, 2020

Choose a reason for hiding this comment

github-actions bot Jun 25, 2020

Choose a reason for hiding this comment

github-actions bot Jun 25, 2020

Choose a reason for hiding this comment

kpango commented Jun 25, 2020

vdaas-ci commented Jun 25, 2020 • edited by rinx Loading

rinx commented Jun 23, 2020 •

edited

Loading

pull-assistant bot commented Jun 23, 2020 •

edited

Loading

codecov bot commented Jun 23, 2020 •

edited

Loading

vdaas-ci commented Jun 25, 2020 •

edited by rinx

Loading