[AMORO-3393]: A stateless catalog manager implement #3394
Conversation
baiyangtx
requested review from
klion26 and
zhoujinsong
and removed request for
zhoujinsong
zhoujinsong
left a comment
zhoujinsong
left a comment
There was a problem hiding this comment.
@baiyangtx
Thanks for the contribution!
The PR looks good to me in general.
But I have a concern about the performance, you may want to check.
|
|
||
| @Override | ||
| public List<ExternalCatalog> getExternalCatalogs() { | ||
| return listCatalogMetas().stream() |
There was a problem hiding this comment.
We can improve the performance of this method by reducing some database calls.
Now it will select all the catalog metadata and then select each one of them individually, which should be improved.
|
|
||
| @Override | ||
| public ServerCatalog getServerCatalog(String catalogName) { | ||
| Optional<CatalogMeta> catalogMeta = getCatalogMetaOptional(catalogName); |
There was a problem hiding this comment.
Catalog metadata changes are infrequent. Should we control the minimum query interval to avoid accessing the database too frequently?
There was a problem hiding this comment.
Cache will lead to consistency issues, and unit tests will also fail.
I have optimized this part, and now each call only accesses the database once.
zhoujinsong
left a comment
zhoujinsong
left a comment
There was a problem hiding this comment.
LGTM.
Thanks for the contribution!
* stateless catalog manager * stateless catalog manager * ci * ci * improve db access * add catalog meta cache * add catalog meta cache * ut * ut --------- Co-authored-by: zhangyongxiang.alpha <[email protected]>
2 participants
Why are the changes needed?
Close #3393.
This is a subtask to implement a stateless RESTController.
Brief change log
How was this patch tested?
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before making a pull request
Documentation