feat(cache): cache metadata location in signer #334
Conversation
twuebi
force-pushed
the
tp/cache-metadata-location
branch
from
dc7413a
to
5700923
Compare
twuebi
force-pushed
the
tp/cache-metadata-location
branch
from
c60b3c0
to
ff05602
Compare
twuebi
force-pushed
the
tp/cache-metadata-location
branch
from
ff05602
to
161a98e
Compare
twuebi
force-pushed
the
tp/cache-metadata-location
branch
from
35d0521
to
96ff1c4
Compare
twuebi
force-pushed
the
tp/cache-metadata-location
branch
from
96ff1c4
to
2d67ea4
Compare
| @@ -2,6 +2,7 @@ | |||
| use anyhow::anyhow; | |||
| use std::collections::HashSet; | |||
| use std::convert::Infallible; | |||
| use std::num::NonZeroUsize; | |||
There was a problem hiding this comment.
it's somewhat of a pain to work with.. there's no const constructor which would be infallible that I found
| .r#type("CreateTableLocationRequired".to_string()) | ||
| .build() | ||
| })? | ||
| .trim_end_matches('/') |
There was a problem hiding this comment.
This shouldn't be part of the Postgres impl, but fixed in catalog.rs so that its fixed for all backends.
There was a problem hiding this comment.
I'm also adding it to determine_tabular_location, it's also here since most tests do not create tables via the catalog but instead directly on postgres impl level
| @@ -334,79 +337,84 @@ pub(crate) async fn get_table_metadata_by_id( | |||
| }) | |||
| } | |||
|
|
|||
| pub(crate) async fn get_table_metadata_by_s3_location( | |||
| pub(crate) async fn get_table_id_by_s3_location( | |||
| warehouse_id: WarehouseIdent, | |||
| location: &str, | |||
There was a problem hiding this comment.
Could we change the type to configs::Location (basically URL?).
I think just by splitting location at :// it is not guaranteed to be a valid URL. Location type is a bit more typesafe and allows easier access to protocol and (sub)-paths by path_segemnts.
We might need to expose more attributes of Location.
Maybe having sublocation part of configs.Location makes sense? Then we could share it across impls again.
There was a problem hiding this comment.
changed to use location instead
Adds a cache for get_table_metadata by location, we're only caching the table_id since that's not going to change.
partially addresses #6