storage: extend the `ExternalStorage` interface and implements by glorv · Pull Request #446 · pingcap/br

glorv · 2020-07-30T07:39:07Z

What problem does this PR solve?

extend the ExternalStorage interface and implement the new methods for local and s3 storage. This extension is currently used in lightning to support restore from s3 storage.

What is changed and how it works?

Add new methods for ExternalStorage:

type ReadSeekCloser interface {
	io.Reader
	io.Seeker
	io.Closer
}

type ExtrenalStorage interface {
	// Open a Reader by file name
	Open(ctx context.Context, name string) (ReadSeekCloser, error)
	// WalkDir traverse all the files in a dir
	WalkDir(ctx context.Context, fn func(string, int64) error) error
}

Since s3 GetObject does not provide seek directly, we implement this by: 1) read and skip the bytes if the distance is small, else we open a new reader set the start position to the seek position.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Code changes

Has exported function/method change
Has exported variable/fields change
Has interface methods change

Side effects

Related changes

Release Note

No release note

pkg/storage/s3.go

pkg/storage/gcs.go

glorv · 2020-07-31T02:55:44Z

/run-all-tests

glorv · 2020-08-03T03:15:52Z

/run-all-tests

kennytm · 2020-08-03T07:34:51Z

pkg/storage/flags.go

 func DefineFlags(flags *pflag.FlagSet) {
-	defineS3Flags(flags)
+	DefineS3Flags(flags)
 	defineGCSFlags(flags)
 }


what is the rationale of only exposing S3 but not GCS?

glorv · 2020-08-03T09:31:35Z

@kennytm Shall we change the s3 and gcs flag definitions from pflag to flag so that lightning can share the same definitions without migrating into pflag?

kennytm · 2020-08-03T10:18:41Z

@glorv we don't really need those flags since they can be passed via URL parameters (s3://bucket/prefix?region=us-east-1 etc).

Given that pflag refuses to support -long-options, migrating Lightning to pflag requires approval of products dept.

overvenus · 2020-08-04T08:41:05Z

pkg/storage/s3.go

+	// FIXME: we test in minio, when request with Range, the result.AcceptRanges is a bare string 'range',
+	//  not sure whether this is a feature or bug
+	//if rangeOffset != nil && (result.AcceptRanges == nil || *result.AcceptRanges != *rangeOffset) {
+	//	return nil, errors.Errorf("open file '%s' failed, expected range: %s, got: %v",
+	//		name, *rangeOffset, result.AcceptRanges)
+	//}


Have we tested in AWS S3?

overvenus · 2020-08-04T08:43:25Z

pkg/storage/local.go

+// export for test.
+func NewLocalStorage(base string) (*LocalStorage, error) {


Where is the test?

used in lightning tests

overvenus · 2020-08-04T08:46:33Z

pkg/storage/s3.go

+		return realOffset, nil
+	}
+
+	// if seek ahead no more than 64k, we read add drop these data


Suggested change

// if seek ahead no more than 64k, we read add drop these data

// if seek ahead no more than 64k, we discard these data

overvenus · 2020-08-04T08:48:18Z

pkg/storage/s3.go

+		return 0, err
+	}
+
+	newReader, err := r.storage.open(context.Background(), r.name, realOffset, 0)


Please avoid use Background context.

glorv · 2020-08-10T09:54:26Z

/run-all-tests

glorv · 2020-08-10T10:35:07Z

/run-all-tests

overvenus · 2020-08-11T06:35:05Z

LGTM

overvenus · 2020-08-11T06:35:11Z

/run-all-tests

kennytm

LGTM

overvenus · 2020-08-13T06:33:19Z

/merge

ti-srebot · 2020-08-13T06:35:48Z

/run-all-tests

…ap#446) * update storage * update * fix * export s3 flags * fix local path * fix s3 reader * fix range offset * fix the req range errors * fix comments * fix review comments * fix some comments * update * fix * update Co-authored-by: ti-srebot <66930949+ti-srebot@users.noreply.github.com>

…#462) * update storage * update * fix * export s3 flags * fix local path * fix s3 reader * fix range offset * fix the req range errors * fix comments * fix review comments * fix some comments * update * fix * update Co-authored-by: ti-srebot <66930949+ti-srebot@users.noreply.github.com> Co-authored-by: ti-srebot <66930949+ti-srebot@users.noreply.github.com>

glorv added 8 commits July 28, 2020 16:21

update storage

9ee6dda

update

4b40812

fix

043e0b3

export s3 flags

568b53a

fix local path

3cc77d5

fix s3 reader

f7e38f3

fix range offset

fb34eff

fix the req range errors

6241ed3

glorv requested review from 3pointer and kennytm July 30, 2020 07:39

fix comments

0acd776

kennytm reviewed Jul 30, 2020

View reviewed changes

pkg/storage/s3.go Outdated Show resolved Hide resolved

pkg/storage/s3.go Outdated Show resolved Hide resolved

pkg/storage/s3.go Outdated Show resolved Hide resolved

pkg/storage/gcs.go Show resolved Hide resolved

fix review comments

c259509

glorv mentioned this pull request Jul 30, 2020

restore: support restore from s3 pingcap/tidb-lightning#361

Merged

kennytm reviewed Aug 3, 2020

View reviewed changes

overvenus reviewed Aug 4, 2020

View reviewed changes

glorv added 5 commits August 10, 2020 15:13

Merge branch 'master' of https://github.com/pingcap/br into storage

2c76f67

fix some comments

1f04897

update

1d52244

fix

c911dc8

update

1b67827

ti-srebot added the status/LGT1 LGTM1 label Aug 11, 2020

Merge branch 'master' into storage

a3919d2

kennytm reviewed Aug 11, 2020

View reviewed changes

ti-srebot removed the status/LGT1 LGTM1 label Aug 11, 2020

ti-srebot approved these changes Aug 11, 2020

View reviewed changes

ti-srebot added the status/LGT2 LGTM2 label Aug 11, 2020

ti-srebot added the CanMerge label Aug 13, 2020

Merge branch 'master' into storage

c8e6173

ti-srebot merged commit 1ab8511 into pingcap:master Aug 13, 2020

glorv mentioned this pull request Aug 17, 2020

storage: extend the ExternalStorage interface and implements (#446) #462

Merged

kennytm mentioned this pull request Oct 5, 2020

storage: S3 test refactoring and fix some bugs #548

Merged

glorv deleted the storage branch April 8, 2021 04:05

		// export for test.
		func NewLocalStorage(base string) (*LocalStorage, error) {

	// if seek ahead no more than 64k, we read add drop these data
	// if seek ahead no more than 64k, we discard these data

Conversation

glorv commented Jul 30, 2020

What problem does this PR solve?

What is changed and how it works?

Check List

Release Note

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glorv commented Jul 31, 2020

Uh oh!

glorv commented Aug 3, 2020

Uh oh!

kennytm Aug 3, 2020

Choose a reason for hiding this comment

Uh oh!

glorv commented Aug 3, 2020

Uh oh!

kennytm commented Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

overvenus Aug 4, 2020

Choose a reason for hiding this comment

Uh oh!

overvenus Aug 4, 2020

Choose a reason for hiding this comment

Uh oh!

glorv Aug 5, 2020

Choose a reason for hiding this comment

Uh oh!

overvenus Aug 4, 2020

Choose a reason for hiding this comment

Uh oh!

overvenus Aug 4, 2020

Choose a reason for hiding this comment

Uh oh!

glorv commented Aug 10, 2020

Uh oh!

glorv commented Aug 10, 2020

Uh oh!

overvenus commented Aug 11, 2020

Uh oh!

overvenus commented Aug 11, 2020

Uh oh!

kennytm left a comment

Choose a reason for hiding this comment

Uh oh!

overvenus commented Aug 13, 2020

Uh oh!

ti-srebot commented Aug 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kennytm commented Aug 3, 2020 •

edited

Loading