Migrate metastore to code generated metastore service #3898
Conversation
fmassot
changed the title
fmassot
force-pushed
the
fmassot/migrate-codegen-metastore
branch
2 times, most recently
from
a4d7a3e to
baebca5
Compare
fmassot
force-pushed
the
fmassot/migrate-codegen-metastore
branch
10 times, most recently
from
c665a7f to
1e66102
Compare
fmassot
force-pushed
the
fmassot/migrate-codegen-metastore
branch
6 times, most recently
from
ee70464 to
ca832cc
Compare
fmassot
force-pushed
the
fmassot/migrate-codegen-metastore
branch
from
ca832cc to
148460a
Compare
fmassot
changed the title
| .await; | ||
| let delete_splits_request = DeleteSplitsRequest { | ||
| index_uid: index_uid.to_string(), | ||
| split_ids: split_ids.clone(), |
There was a problem hiding this comment.
Are you sure we need to clone?
There was a problem hiding this comment.
We can remove the error message, is that what you mean?
I'm fine with removing it, we return the appropriate metastore error that we could log instead.
fmassot
force-pushed
the
fmassot/migrate-codegen-metastore
branch
3 times, most recently
from
77c80df to
89e8c1a
Compare
| cache: Arc<Mutex<HashMap<Uri, Weak<dyn Metastore>>>>, | ||
| // We never garbage collect unused metastore client instances. This should not be a problem | ||
| // because during normal use this cache will hold at most a single instance. | ||
| cache: Arc<Mutex<HashMap<Uri, MetastoreServiceClient>>>, |
There was a problem hiding this comment.
If this is true, maybe we should use an Arc<Mutex<Option<(Uri, MetastoreServiceClient)>>> and always evict the old client if URIs don't match (and log a warning?)
There was a problem hiding this comment.
I like the idea and I tested it. Only a few tests are failing if we changed this. We need to also to refactor the factory as now the code between the file backed factory cache and the postgres factory cache is exactly the same.
quickwit/quickwit-metastore/src/metastore/postgresql_metastore.rs
Outdated
Show resolved
Hide resolved
| storage_resolver: &StorageResolver, | ||
| index_id: &str, | ||
| source_config_opt: Option<&SourceConfig>, | ||
| ) -> anyhow::Result<()> { | ||
| let mut checks: Vec<(&str, anyhow::Result<()>)> = Vec::new(); | ||
|
|
||
| // The metastore is file-backed, so we must check the storage first. |
There was a problem hiding this comment.
This check is useless. The check connectivity of the metastore will do the right check.
There was a problem hiding this comment.
Leave it. The checklist will tell you it's a metastore issue, whereas it stems from the storage.
There was a problem hiding this comment.
Ok, will change the comment then.
There was a problem hiding this comment.
I would prefer to do the right check in the metastore method and make sure the error displayed is clean.
fmassot
force-pushed
the
fmassot/migrate-codegen-metastore
branch
from
0f530a6 to
4c43b9b
Compare
fmassot
force-pushed
the
fmassot/migrate-codegen-metastore
branch
from
dc1abe3 to
7655fb0
Compare
TODO
IndexUidcheck_connectivityandurismethods (add a new traitMetastoreServiceExt?)index_existsmethodMetastoreServiceClient::new(mock_metastore), but we need to wrap the mock. Also it's possible to doMockMetastoreService::new()and I think we don't want a developer to do that => I added an assert if a dev instantiate withMetastoreServiceClient::newinstead ofMetastoreServiceClient::from.Some proto requests have aThe solution would be to have only optional fields....defaultimplementation that will generate an empty json payload, this is badNext steps:
Service<R>for each metastore request on the control plane client. I don't have a concrete plan on this and prefer to open an issue to fix it later. I may miss an obvious solution, let's discuss that tomorrow.ListSplitsRequest::try_from_index_uidbyListSplitsRequest::from_index_uidas we are sure this builder won't fail.