Write directly to S3

[tirsen]

What problem does this PR solve?

#8 Support S3

What is changed and how it works?

Switch to using VFS which supports local files, S3 and GCS. For now this only supports S3 but it should be easy to add support to e.g. GCS.

You simply prefix the output directory with "s3://bucket" e.g.:

dumpling -o s3://mybycket/dir

For now all config is passed in using the standard AWS environment variables.

Check List

Tests

• Unit test
• Integration test
• Manual test (add detailed scripts or steps below)
  Will test by running dumpling against our staging databases.

Side effects

• Introduces a new dependency on github.com/c2fo/vfs

Related changes

• Need to update the documentation

Release note

• support writing directly to S3

[CLAassistant]

All committers have signed the CLA.

[kennytm]

Hi @tirsen, thanks for the contribution! However, we're more interested in including S3 support through the github.com/pingcap/br/pkg/storage shared by all migration tools, which we can also get GCS support for free. Could you see if it's possible to rewrite this using the ExternalStorage interface?

[tirsen]

It's not trivial. Afaict there is no Writer implementation which means I would have to implement one that buffers and then sends a chunk at a time using the Uploader. All doable of course but, yeah, not trivial. :-)

I could implement a Writer on top of ExternalStorage or is there one already?

[tirsen]

Dumpling already does some level of "buffering" itself for example here but the buffer size is hard coded to be 1mb which is not necessarily optimal for S3. It needs to be configurable. It would be better to implement buffering at the github.com/pingcap/br/pkg/storage layer so that the configuration option for chunk size is consistent across clients.

[tirsen]

Actually vfs isn't a good choice for this use case anyway, it buffers the entire content of a file in memory and sends all of it on close: https://github.com/C2FO/vfs/blob/master/backend/s3/file.go#L203

Or does dumpling split up tables into many smaller files? How large does individual files become if we dump say a 300gb large database?

[kennytm]

@tirsen

I could implement a Writer on top of ExternalStorage or is there one already?

Yes you could implement a Writer on top of ExternalStorage

Dumpling already does some level of "buffering" itself for example here but the buffer size is hard coded to be 1mb which is not necessarily optimal for S3. It needs to be configurable. It would be better to implement buffering at the github.com/pingcap/br/pkg/storage layer so that the configuration option for chunk size is consistent across clients.

Yes!

Or does dumpling split up tables into many smaller files? How large does individual files become if we dump say a 300gb large database?

If you supply -F or -r, dumpling do split up the table into smaller files according to file size or row count.

But if none of the argument is provided then yes Dumpling will happily create a 300 GB file.

[tirsen]

@kennytm Here is the Writer implementation on top of Uploader: pingcap/br#524

[tirsen]

Ok I think this is good to go now

[lichunzhu]

Nice job! We will take a look soon.

[tirsen]

Ok I think all comments resolved now. PHAL

[lichunzhu]

Can we add a configuration region to set region for s3 configurations? When I'm testing upload to S3, setting AWS_REGION doesn't work. If config.BackendOptions.S3.Region is set it will work correctly.
Rest LGTM, @kennytm PTAL

[kennytm]

@lichunzhu using the URL s3://bucket/path?region=us-west-2 should work.

[lichunzhu]

@lichunzhu using the URL s3://bucket/path?region=us-west-2 should work.

Yes it works. So we just ask users to set region in the OutputDir URL?

[tirsen]

CLI flags are now exposed so you can also set the region that way. Is AWS_REGION a standard env var? Should we support that in the underlying br library?

[kennytm]

Is AWS_REGION a standard env var?

This env var should be used by default, but unfortunately we have overridden the config making the env var useless.

I think we should fix this later on BR.

[kennytm]

rest LGTM, thanks 😄