feat: Enable GCS, S3, R2 and MinIO as object stores for local runs #1843

gruuya · 2023-09-27T19:00:52Z

This PR introduces support for using external object stores as data/catalog storage in locally run GlareDB.

Still a couple of things missing:

Test coverage
Read creds from user's home

Closes #1806, and also partially addresses #1818.

crates/glaredb/src/args/mod.rs

crates/sqlexec/src/engine.rs

gruuya · 2023-09-28T09:03:52Z

Verified it works on GCS:

$ export GOOGLE_SERVICE_ACCOUNT=/Users/markogrujic/GlareDB/glaredb-dev-playground-99fdd8e83a7c.json
$ cargo run --bin glaredb -- -l gs://gruuya-data-store-test
warning: profiles for the non root package will be ignored, specify profiles at the workspace root:
package:   /Users/markogrujic/GlareDB/glaredb/py-glaredb/Cargo.toml
workspace: /Users/markogrujic/GlareDB/glaredb/Cargo.toml
    Finished dev [unoptimized + debuginfo] target(s) in 0.66s
     Running `target/debug/glaredb -l 'gs://gruuya-data-store-test'`
GlareDB (v0.5.0)
Using in-memory catalog
Type \help for help.
> create table test as values (1, 'one'), (2, 'two');
Table created
> select * from test where column1 = 2;
┌─────────┬─────────┐
│ column1 │ column2 │
│      ── │ ──      │
│   Int64 │ Utf8    │
╞═════════╪═════════╡
│       2 │ two     │
└─────────┴─────────┘

Resulting delta table logs:

@scsmithr @vrongmeal @tychoish as for the S3 family of object stores there's a matter of deciding what to do with atomic copy-if-not-exists capabilities required for leasing. Cloudflare R2 has a header that enables this action: https://docs.rs/object_store/latest/object_store/aws/enum.S3CopyIfNotExists.html

For others (basically Amazon proper and MinIO) I see the options (not necessarily exclusive) as:

do a non-atomic rename in two steps (check if the lease doesn't exist and then rename); this would leave us vulnerable to races, which would need to be clearly communicated to the users
enable configuring an S3 lock client (like in delta-rs); this would provide the needed atomicity, though it would introduce additional complexity (particularly in the CLI, might as well implement a glare config file at that point)

crates/glaredb/src/args/mod.rs

vrongmeal · 2023-09-28T09:14:54Z

crates/metastore/src/local.rs

@@ -27,8 +27,7 @@ pub async fn start_inprocess_local(

 /// Starts an in-process metastore service, returning a client for the service.
 ///
-/// Useful for some tests, as well as when running GlareDB locally for testing.
-/// This should never be used in production.


crates/sqlexec/src/engine.rs

scsmithr · 2023-09-28T12:52:29Z

@scsmithr @vrongmeal @tychoish as for the S3 family of object stores there's a matter of deciding what to do with atomic copy-if-not-exists capabilities required for leasing. Cloudflare R2 has a header that enables this action: https://docs.rs/object_store/latest/object_store/aws/enum.S3CopyIfNotExists.html

For others (basically Amazon proper and MinIO) I see the options (not necessarily exclusive) as:
* do a non-atomic rename in two steps (check if the lease doesn't exist and then rename); this would leave us vulnerable to races, which would need to be clearly communicated to the users

* enable configuring an S3 lock client (like in delta-rs); this would provide the needed atomicity, though it would introduce additional complexity (particularly in the CLI, might as well implement a glare config file at that point)

The first option would be preferred to avoid having external system dependencies.

What I think we can do is change lease initialization logic to:

Read lease. If it exists:

return Ok if process id written in lease matches leaser's process id
return Err if process ids don't match

Put new lease (puts are atomic in s3)
Read lease again to ensure no other process wrote the lease between steps 1 & 2.

Note that lease initialization does not automatically acquire the lease, and so an additional call to acquire is made. Internally this will do an additional read on the lease object to ensure the process ids match, then that starts up a background loop to keep the lease updated, erroring if some other process came in and messed with the lease.

scsmithr · 2023-09-28T12:53:38Z

Not requiring copy_if_not_exists would also help with #1809

tychoish · 2023-09-28T18:14:15Z

For others (basically Amazon proper and MinIO) I see the options (not necessarily exclusive) as:

do a non-atomic rename in two steps (check if the lease doesn't exist and then rename); this would leave us vulnerable to races, which would need to be clearly communicated to the users

enable configuring an S3 lock client (like in delta-rs); this would provide the needed atomicity, though it would introduce additional complexity (particularly in the CLI, might as well implement a glare config file at that point)

Is there a third option that's "indirect [some?] names (or append random keys when writing to avoid clashes?)"

Is there a fourth option that's do naming/renaming (again of some level?) as a (relatively simple) two phase commit?

I think we have to avoid clashing with ourselves more than anything: I don't think we need to solve "how do we avoid breaking things/being broken when someone manually moves files around while we're writing data.

Am I misunderstanding things?

crates/object_store_util/src/shared.rs

vrongmeal · 2023-10-02T12:03:45Z

crates/metastore/Cargo.toml

@@ -6,6 +6,7 @@ edition = {workspace = true}
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

 [dependencies]
+anyhow = "1.0"


Minor nitpick, but, we would want to add errors to the enum instead of using anyhow in this crate.

Oh I see; changing, thanks.

vrongmeal · 2023-10-02T12:04:27Z

crates/object_store_util/Cargo.toml

@@ -6,6 +6,7 @@ edition = { workspace = true }
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

 [dependencies]
+anyhow = "1.0"


Same comment as for metastore.

tychoish · 2023-10-02T13:29:27Z

crates/glaredb/src/args/mod.rs

+    pub storage_options: Vec<(String, String)>,
+}
+
+fn parse_key_value_pair(key_value_pair: &str) -> Result<(String, String)> {


what if you specify options which aren't supported, or options that are specified more than once? will order matter (should we sort by key?)

While I think it's good to have some kind of flexible configs here (given that not all backing storage is going have the same arguments),

it's my assumption that we'll (eventually) have all this specified by a config file or most people will specify the options interactively in via SQL/python.

what if you specify options which aren't supported

Unsupported options will be ignored but the initialization/setup will error out with a clear message (either from us, or courtesy of the object store crate itself) if some of the required keys are missing.

or options that are specified more than once? will order matter (should we sort by key?)

The last option value will be used in this case.

tychoish · 2023-10-02T13:42:32Z

crates/glaredb/src/args/local.rs

@@ -35,6 +35,9 @@ pub struct LocalClientOpts {
    #[clap(short = 'c', long, value_parser)]
    pub cloud_url: Option<Url>,

+    #[clap(flatten)]


I kept wanting there to be soemthing like "flatten with prefix" so we could still get strict parsing that we didn't have to write ourselves, but it seems like this isn't to be. I also sort of wanted ArgGroup to be able to get us more parsing, but again, no such luck.

crates/metastore/src/errors.rs

tychoish · 2023-10-02T13:57:49Z

crates/sqlexec/src/engine.rs

 /// Storage configuration for the compute engine.
 ///
 /// The configuration defined here alongside the configuration passed in through
 /// the proxy will be used to connect to database storage.
 #[derive(Debug, Clone)]
 pub enum EngineStorageConfig {
-    Gcs { service_account_key: String },
-    Local { path: PathBuf },
+    S3 {


does it make sense to split into storage config into authentication components which are provider specific and general options (e.g. bucket name, path prefix, etc?)

also does it make sense to have a "path prefix" so that you could theoretically store multiple engines one one bucket?

does it make sense to split into storage config into authentication components

Fwiw, I did try this initially; will try to change to that setup now as well.

also does it make sense to have a "path prefix"

Good question; usually in these cases the bucket parameter can hold an arbitrary path as well, so I figured that would work, but after a quick test now I see it doesn't so I'll need to investigate/fix that.

I think we should get that into main before cutting a release, but can merge this PR and then add prefixes in, if you want.

Ok, will go ahead with merge, as I have other follow-up work as well (i.e. functional tests for this feature). Opened #1873 to track those two.

…ocal client

tychoish · 2023-10-02T21:30:57Z

crates/sqlexec/src/engine.rs

                                    )
                            });

                        let service_account_key = fs::read_to_string(service_account_path)?;

-                        let bucket = bucket.or(opts.get("bucket").cloned());
+                        // Buket potentially provided as a part of the location URL, try to extract it.


Suggested change

// Buket potentially provided as a part of the location URL, try to extract it.

// Bucket potentially provided as a part of the location URL, try to extract it.

tychoish · 2023-10-02T22:11:09Z

crates/sqlexec/src/engine.rs

 /// Storage configuration for the compute engine.
 ///
 /// The configuration defined here alongside the configuration passed in through
 /// the proxy will be used to connect to database storage.
 #[derive(Debug, Clone)]
 pub enum EngineStorageConfig {
-    Gcs { service_account_key: String },
-    Local { path: PathBuf },
+    S3 {


I think we should get that into main before cutting a release, but can merge this PR and then add prefixes in, if you want.

tychoish

(just for the record I think this is mergable modulo the changes discussed)

Enable GCS, S3, R2 and MinIO as object stores for locall runs

a5a547e

gruuya requested review from scsmithr, universalmind303, tychoish and vrongmeal September 27, 2023 19:17

gruuya commented Sep 28, 2023

View reviewed changes

crates/glaredb/src/args/mod.rs Outdated Show resolved Hide resolved

crates/sqlexec/src/engine.rs Show resolved Hide resolved

gruuya added 2 commits September 28, 2023 07:34

Merge branch 'main' into remote-data-object-storage

affa887

Parse bucket from provided location URL in local runs

eb6f680

gruuya changed the title ~~feat: Enable GCS, S3, R2 and MinIO as object stores for locall runs~~ feat: Enable GCS, S3, R2 and MinIO as object stores for local runs Sep 28, 2023

vrongmeal reviewed Sep 28, 2023

View reviewed changes

gruuya added 2 commits September 28, 2023 13:52

Fix CLI param naming and enable atomic renaming for CF R2

e806edb

Fix the location cli argument leaking into other commands

9a319ba

gruuya added 3 commits September 29, 2023 10:20

Implement non-atomic copy-if-not-exists for the shared object store

90d8cb1

Merge branch 'main' into remote-data-object-storage

9a9a0d2

cargo fmt the SharedObjectStore changes

7749af3

gruuya commented Sep 29, 2023

View reviewed changes

crates/object_store_util/src/shared.rs Outdated Show resolved Hide resolved

gruuya marked this pull request as ready for review September 29, 2023 15:24

gruuya requested a review from adhish20 September 29, 2023 15:57

gruuya added 4 commits September 29, 2023 17:58

Merge branch 'main' into remote-data-object-storage

a1125d5

Simplify the long name for an individual storage option arg

c266dfe

Merge branch 'main' into remote-data-object-storage

f90b8af

Enable passing storage configuration via python bindings

5e01e31

vrongmeal reviewed Oct 2, 2023

View reviewed changes

Remove anyhow errors from metastore and object_store_util crates

66286fa

scsmithr approved these changes Oct 2, 2023

View reviewed changes

tychoish reviewed Oct 2, 2023

View reviewed changes

gruuya added 2 commits October 2, 2023 16:56

Use an existing metastore error when failing to start an in-process l…

7ce3492

…ocal client

Merge branch 'main' into remote-data-object-storage

578b4f5

tychoish reviewed Oct 2, 2023

View reviewed changes

tychoish approved these changes Oct 2, 2023

View reviewed changes

gruuya added 2 commits October 3, 2023 09:13

Merge branch 'main' into remote-data-object-storage

453990b

Fix typo in comment

13d1583

gruuya self-assigned this Oct 3, 2023

gruuya enabled auto-merge (squash) October 3, 2023 07:21

gruuya merged commit c3cc95a into main Oct 3, 2023
7 checks passed

gruuya deleted the remote-data-object-storage branch October 3, 2023 07:29

universalmind303 mentioned this pull request Oct 3, 2023

Add support for additional object storages like R2 or minio #1818

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Enable GCS, S3, R2 and MinIO as object stores for local runs #1843

feat: Enable GCS, S3, R2 and MinIO as object stores for local runs #1843

gruuya commented Sep 27, 2023 •

edited

Loading

gruuya commented Sep 28, 2023 •

edited

Loading

vrongmeal Sep 28, 2023

scsmithr commented Sep 28, 2023

scsmithr commented Sep 28, 2023

tychoish commented Sep 28, 2023

vrongmeal Oct 2, 2023

gruuya Oct 2, 2023

vrongmeal Oct 2, 2023

tychoish Oct 2, 2023

gruuya Oct 2, 2023

tychoish Oct 2, 2023

tychoish Oct 2, 2023

gruuya Oct 2, 2023

tychoish Oct 2, 2023

gruuya Oct 3, 2023

tychoish Oct 2, 2023

tychoish Oct 2, 2023

tychoish left a comment

	// Buket potentially provided as a part of the location URL, try to extract it.
	// Bucket potentially provided as a part of the location URL, try to extract it.

feat: Enable GCS, S3, R2 and MinIO as object stores for local runs #1843

feat: Enable GCS, S3, R2 and MinIO as object stores for local runs #1843

Conversation

gruuya commented Sep 27, 2023 • edited Loading

gruuya commented Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

scsmithr commented Sep 28, 2023

scsmithr commented Sep 28, 2023

tychoish commented Sep 28, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tychoish left a comment

Choose a reason for hiding this comment

gruuya commented Sep 27, 2023 •

edited

Loading

gruuya commented Sep 28, 2023 •

edited

Loading