feat: Extend SLTs to excercise remote data storage functionality #1874
Conversation
.github/workflows/ci.yaml
Outdated
| --option access_key_id=$MINIO_ACCESS_KEY \ | ||
| --option secret_access_key=$MINIO_SECRET_KEY \ | ||
| --option bucket=$MINIO_BUCKET \ | ||
| 'sqllogictests_native/*' |
There was a problem hiding this comment.
We probably want to include a couple more tests that are heavy on native DDL & DML statements, so as to exercise the remote object store as much as possible. What would be some good paths to add here?
There was a problem hiding this comment.
I think we should run the sqllogictests/* tests as well using this. Most of the DDL/DML tests lie there!
There was a problem hiding this comment.
Great, thanks for the info!
| @@ -43,6 +43,9 @@ pub struct ServerArgs { | |||
| #[clap(short, long, hide = true, value_parser)] | |||
| pub service_account_path: Option<String>, | |||
|
|
|||
| #[clap(flatten)] | |||
| pub storage_config: StorageConfigArgs, | |||
There was a problem hiding this comment.
hide doesn't play nicely with flatten it seems, so this will appear on help invocations.
There was a problem hiding this comment.
Not the worst thing. I started adding in hide just so users wouldn't be confused about options that they shouldn't be setting (e.g. segment keys).
| @@ -69,6 +69,8 @@ fn main() -> Result<()> { | |||
| None, | |||
| None, | |||
| None, | |||
| Default::default(), | |||
| None, | |||
There was a problem hiding this comment.
This function getting intense with all the Options.
There was a problem hiding this comment.
Yeah; I think there's a remote chance we can deprecate/consolidate data_dir and service_account_key into storage config so we'll be back to previous situation.
| @@ -43,6 +43,9 @@ pub struct ServerArgs { | |||
| #[clap(short, long, hide = true, value_parser)] | |||
| pub service_account_path: Option<String>, | |||
|
|
|||
| #[clap(flatten)] | |||
| pub storage_config: StorageConfigArgs, | |||
There was a problem hiding this comment.
Not the worst thing. I started adding in hide just so users wouldn't be confused about options that they shouldn't be setting (e.g. segment keys).
scripts/create-test-minio-store.sh
Outdated
| docker run --rm --net=host --entrypoint=/bin/sh -i minio/mc:latest <<EOF | ||
| #!/usr/bin/mc | ||
|
|
||
| # Wait for minio server to become ready | ||
| curl --retry 10 -f --retry-connrefused --retry-delay 1 http://localhost:9000/minio/health/live | ||
|
|
||
| # Configure mc to connect to our above container as host | ||
| mc config host add glaredb_minio http://localhost:9000 $MINIO_ACCESS_KEY $MINIO_SECRET_KEY | ||
|
|
||
| # Remove the bucket if it already exists | ||
| mc rm -r --force glaredb_minio/"$MINIO_BUCKET" | ||
|
|
||
| # Finally create the test bucket | ||
| mc mb glaredb_minio/"$MINIO_BUCKET" | ||
| EOF |
There was a problem hiding this comment.
what's mc and does writing to stdin of /bin/sh respect a shebang? (I have no idea!) It's probably the case that you're not calling mc from within the mc` shell....
There was a problem hiding this comment.
Good catch, that's just a leftover from my previous attempt to get this working. Removing now.
If by "jobs" you mean "tasks" in the workflow file, I feel great about it... I think we would need to make the current
|
There was a problem hiding this comment.
I think this is fine to merge in its current form; I think in the medium term we could/should do a little cleanup with our integration test orchestration, but we don't need to block this PR on that (though @vrongmeal or @gruuya can/should coordinate on some of those details.)
The simplest way to go about this seemed to be:
ci.yamlfile to trigger all that