Hi Feng, This is a useful FLIP. Thanks for starting this discussion. The current design looks pretty good to me. I just have some minor comments.
1. How to register the CatalogStore for Table API? I think the CatalogStore should be immutable once TableEnv is created. Otherwise, there might be some data inconsistencies when CatalogStore is changed. 2. Why does the CatalogStoreFactory interface only have a default method, not an interface method? 3. Please mention the alternative API in Javadoc for the deprecated `registerCatalog`. 4. In the "Compatibility" section, would be better to mention the changed behavior of CREATE CATALOG statement if FileCatalogStore (or other persisted catalog store) is used. Best, Jark On Thu, 1 Jun 2023 at 11:26, Feng Jin <jinfeng1...@gmail.com> wrote: > Hi ， thanks all for reviewing the flip. > > @Ron > > > Regarding the CatalogStoreFactory#createCatalogStore method, do we need > to provide a default implementation? > > Yes, we will provide a default InMemoryCatalogStoreFactory to create an > InMemoryCatalogStore. > > > If we get a Catalog from CatalogStore, after initializing it, whether we > put it in Map<String, Catalog> catalogs again? > > Yes, in the current design, catalogs are stored as snapshots, and once > initialized, the Catalog will be placed in the Map<String, Catalog> > catalogs. > Subsequently, the Map<String, Catalog> catalogs will be the primary source > for obtaining the corresponding Catalog. > > > how about renaming them to `catalog.store.type` and > `catalog.store.path`? > > I think it is okay. Adding "sql" at the beginning may seem a bit strange. I > will update the FLIP. > > > > @Shammon > > Thank you for the review. I have made the necessary corrections. > Regarding the modifications made to the Public Interface, I have also > included the relevant changes to the `TableEnvironment`. > > > Best, > Feng > > > On Wed, May 31, 2023 at 5:02 PM Shammon FY <zjur...@gmail.com> wrote: > > > Hi feng, > > > > Thanks for updating, I have some minor comments > > > > 1. The modification of `CatalogManager` should not be in `Public > > Interfaces`, it is not a public interface. > > > > 2. `@PublicEvolving` should be added for `CatalogStore` and > > `CatalogStoreFactory` > > > > 3. The code `Optional<Catalog> optionalDescriptor = > > catalogStore.get(catalogName);` in the `CatalogManager` should be > > `Optional<CatalogDescriptor> optionalDescriptor = > > catalogStore.get(catalogName);` > > > > Best, > > Shammon FY > > > > > > On Wed, May 31, 2023 at 2:24 PM liu ron <ron9....@gmail.com> wrote: > > > > > Hi, Feng > > > > > > Thanks for driving this FLIP, this proposal is very useful for catalog > > > management. > > > I have some small questions: > > > > > > 1. Regarding the CatalogStoreFactory#createCatalogStore method, do we > > need > > > to provide a default implementation? > > > 2. If we get Catalog from CatalogStore, after initializing it, whether > we > > > put it to Map<String, Catalog> catalogs again? > > > 3. Regarding the options `sql.catalog.store.type` and > > > `sql.catalog.store.file.path`, how about renaming them to > > > `catalog.store.type` and `catalog.store.path`? > > > > > > Best, > > > Ron > > > > > > Feng Jin <jinfeng1...@gmail.com> 于2023年5月29日周一 21:19写道： > > > > > > > Hi yuxia > > > > > > > > > But from the code in Proposed Changes, once we register the > Catalog, > > > we > > > > initialize it and open it. right? > > > > > > > > Yes, In order to avoid inconsistent semantics of the original CREATE > > > > CATALOG DDL, Catalog will be directly initialized in registerCatalog > so > > > > that parameter validation can be performed. > > > > > > > > In the current design, lazy initialization is mainly reflected in > > > > getCatalog. If CatalogStore has already saved some catalog > > > configurations, > > > > only initialization is required in getCatalog. > > > > > > > > > > > > Best, > > > > Feng > > > > > > > > On Mon, May 29, 2023 at 8:27 PM yuxia <luoyu...@alumni.sjtu.edu.cn> > > > wrote: > > > > > > > > > Hi, Feng. > > > > > I'm trying to understanding the meaning of *lazy initialization*. > If > > > i'm > > > > > wrong, please correct me. > > > > > > > > > > IIUC, lazy initialization means only you need to access the > catalog, > > > then > > > > > you initialize it. But from the code in Proposed Changes, once we > > > > register > > > > > the Catalog, > > > > > we initialize it and open it. right? > > > > > > > > > > Best regards, > > > > > Yuxia > > > > > > > > > > ----- 原始邮件 ----- > > > > > 发件人: "Jing Ge" <j...@ververica.com.INVALID> > > > > > 收件人: "dev" <dev@flink.apache.org> > > > > > 发送时间: 星期一, 2023年 5 月 29日 下午 5:12:46 > > > > > 主题: Re: [DISCUSS] FLIP 295: Support persistence of Catalog > > > configuration > > > > > and asynchronous registration > > > > > > > > > > Hi Feng, > > > > > > > > > > Thanks for your effort! +1 for the proposal. > > > > > > > > > > One of the major changes is that current design will provide > > > > > Map<String,Catalog> catalogs as a snapshot instead of a cache, > which > > > > means > > > > > once it has been initialized, any changes done by other sessions > will > > > not > > > > > affect it. Point 6 described follow-up options for further > > improvement. > > > > > > > > > > Best regards, > > > > > Jing > > > > > > > > > > On Mon, May 29, 2023 at 5:31 AM Feng Jin <jinfeng1...@gmail.com> > > > wrote: > > > > > > > > > > > Hi all, I would like to update you on the latest progress of the > > > FLIP. > > > > > > > > > > > > > > > > > > Last week, Leonard Xu, HangRuan, Jing Ge, Shammon FY, ShengKai > Fang > > > > and I > > > > > > had an offline discussion regarding the overall solution for > Flink > > > > > > CatalogStore. We have reached a consensus and I have updated the > > > final > > > > > > solution in FLIP. > > > > > > > > > > > > Next, let me briefly describe the entire design: > > > > > > > > > > > > 1. > > > > > > > > > > > > Introduce CatalogDescriptor to store catalog configuration > > similar > > > > to > > > > > > TableDescriptor. > > > > > > 2. > > > > > > > > > > > > The two key functions of CatalogStore - void > storeCatalog(String > > > > > > catalogName, CatalogDescriptor) and CatalogDescriptor > > > > > getCatalog(String) > > > > > > will both use CatalogDescriptor instead of Catalog instance. > > This > > > > way, > > > > > > CatalogStore will only be responsible for saving and > retrieving > > > > > catalog > > > > > > configurations without having to initialize catalogs. > > > > > > 3. > > > > > > > > > > > > The default registerCatalog(String catalogName, Catalog > catalog) > > > > > > function in CatalogManager will be marked as deprecated. > > > > > > 4. > > > > > > > > > > > > A new function registerCatalog(String catalogName, > > > CatalogDescriptor > > > > > > catalog) will be added to serve as the default registration > > > function > > > > > for > > > > > > catalogs in CatalogManager. > > > > > > 5. > > > > > > > > > > > > Map<String,Catalog> catalogs in CataloManager will remain > > > unchanged > > > > > and > > > > > > save initialized catalogs.This means that deletion operations > > from > > > > one > > > > > > session won't synchronize with other sessions. > > > > > > 6. > > > > > > > > > > > > To support multi-session synchronization scenarios for > deletions > > > > later > > > > > > on we should make Map<String,Catalog>catalogs > configurable.There > > > may > > > > > be > > > > > > three possible situations: > > > > > > > > > > > > a.Default caching of all initialized catalogs > > > > > > > > > > > > b.Introduction of LRU cache logic which can automatically > clear > > > > > > long-unused catalogs. > > > > > > > > > > > > c.No caching of any instances; each call to getCatalog > creates a > > > new > > > > > > instance. > > > > > > > > > > > > > > > > > > This is the document for discussion: > > > > > > > > > > > > > > > > > > > > > > > > > > > https://docs.google.com/document/d/1HRJNd4_id7i6cUxGnAybmYZIwl5g1SmZCOzGdUz-6lU/edit > > > > > > > > > > > > This is the final proposal document: > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-295%3A+Support+lazy+initialization+of+catalogs+and+persistence+of+catalog+configurations > > > > > > > > > > > > > > > > > > Thank you very much for your attention and suggestions on this > > > FLIP. A > > > > > > special thanks to Hang Ruan for his suggestions on the entire > > design > > > > and > > > > > > organizing offline discussions. > > > > > > > > > > > > If you have any further suggestions or feedback about this FLIP > > > please > > > > > feel > > > > > > free to share. > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > > > > Feng > > > > > > > > > > > > On Sat, May 6, 2023 at 8:32 PM Jing Ge > <j...@ververica.com.invalid > > > > > > > > wrote: > > > > > > > > > > > > > Hi Feng, > > > > > > > > > > > > > > Thanks for improving the FLIP. It looks good to me. We could > > still > > > > > > > reconsider in the future how to provide more common built-in > > cache > > > > > > > functionality in CatalogManager so that not every CatalogSotre > > > > > > > implementation has to take care of it. > > > > > > > > > > > > > > Best regards, > > > > > > > Jing > > > > > > > > > > > > > > On Thu, May 4, 2023 at 1:47 PM Feng Jin <jinfeng1...@gmail.com > > > > > > wrote: > > > > > > > > > > > > > > > Hi Jing, > > > > > > > > > > > > > > > > Thanks for your reply. > > > > > > > > > > > > > > > > > There might be more such issues. I would suggest you > > > completely > > > > > walk > > > > > > > > through the FLIP again and fix those issues > > > > > > > > > > > > > > > > I am very sorry for my carelessness and at the same time, I > > > greatly > > > > > > > > appreciate your careful review. I have thoroughly checked the > > > > entire > > > > > > FLIP > > > > > > > > and made corrections to these issues. > > > > > > > > > > > > > > > > > > > > > > > > > If I am not mistaken, with the current FLIP design, > > > > > CatalogManager > > > > > > > > could work without Optional CatalogStore being configured. > > > > > > > > > > > > > > > > Yes, in the original design, CatalogStore was not necessary > > > because > > > > > > > > CatalogManager used Map<String, Catalog> catalogs to store > > > catalog > > > > > > > > instances. > > > > > > > > However, this caused inconsistency issues. Therefore, I > > modified > > > > this > > > > > > > part > > > > > > > > of the design and removed Map<String, Catalog> catalogs from > > > > > > > > CatalogManager. > > > > > > > > At the same time, InMemoryCatalog will serve as the default > > > > > > CatalogStore > > > > > > > > to save catalogs in memory and replace the functionality of > > > > > > > > Map<String,Catalog>catalogs. > > > > > > > > The previous plan that kept Map<String,Catalog>catalogs has > > been > > > > > moved > > > > > > to > > > > > > > > Rejected Alternatives. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Best, > > > > > > > > Feng > > > > > > > > > > > > > > > > On Sun, Apr 30, 2023 at 9:03 PM Jing Ge > > > <j...@ververica.com.invalid > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > Hi Feng, > > > > > > > > > > > > > > > > > > There are still many places contain inconsistent content, > > e.g. > > > > > > > > > > > > > > > > > > 1. "Asynchronous registration" is still used. > > > > > > > > > 2. The java comment of the method registerCatalog(String > > > > > catalogName, > > > > > > > > > Catalog catalog) in CatalogManager does not tell what the > > > method > > > > is > > > > > > > > doing. > > > > > > > > > > > > > > > > > > > > > > > > > > > There might be more such issues. I would suggest you > > completely > > > > > walk > > > > > > > > > through the FLIP again and fix those issues. > > > > > > > > > > > > > > > > > > About the inMemoryCatalogStore, do you mean that you will > > build > > > > the > > > > > > > cache > > > > > > > > > functionality in the CatalogStore? This is a very different > > > > design > > > > > > > > concept > > > > > > > > > from what the current FLIP described. If I am not mistaken, > > > with > > > > > the > > > > > > > > > current FLIP design, CatalogManager could work without > > Optional > > > > > > > > > CatalogStore being configured. That is the reason why I > > > mentioned > > > > > in > > > > > > > the > > > > > > > > > last email that the example code wrt the Optional > > CatalogStore > > > is > > > > > not > > > > > > > > > correct. > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > > > Jing > > > > > > > > > > > > > > > > > > On Thu, Apr 27, 2023 at 3:55 PM Feng Jin < > > > jinfeng1...@gmail.com> > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > Hi Jing > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > There are still some NIT issues in the FLIP > > > > > > > > > > > > > > > > > > > > Thank you very much for the careful review. I have > already > > > made > > > > > the > > > > > > > > > > relevant changes. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Speaking of the conflict issues in the multi-instance > > > > > > scenarios, I > > > > > > > > am > > > > > > > > > > not > > > > > > > > > > sure if this is the intended behaviour > > > > > > > > > > > > > > > > > > > > Currently, there are conflicts in multiple scenarios with > > the > > > > > > current > > > > > > > > > > design. I am thinking whether we should remove > 'Map<String, > > > > > > Catalog>' > > > > > > > > and > > > > > > > > > > make Cache the default behavior of InMemoryCatalogStore. > > This > > > > > way, > > > > > > > > users > > > > > > > > > > can implement their own CatalogStore to achieve > > > multi-instance > > > > > > > > > > non-conflicting scenarios. What do you think? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > > Feng > > > > > > > > > > > > > > > > > > > > On Thu, Apr 27, 2023 at 9:03 PM Jing Ge > > > > > <j...@ververica.com.invalid > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Hi Feng, > > > > > > > > > > > > > > > > > > > > > > Thanks for working on the FLIP. There are still some > NIT > > > > issues > > > > > > in > > > > > > > > the > > > > > > > > > > FLIP > > > > > > > > > > > like: > > > > > > > > > > > > > > > > > > > > > > 1. Optional<CatalogStore> catalogStore has been used as > > > > > > > CatalogStore > > > > > > > > > > > instead of Optional in the code example. It should be > > fine > > > to > > > > > use > > > > > > > it > > > > > > > > as > > > > > > > > > > > pseudo code for now and update it after you submit the > > PR. > > > > > > > > > > > 2. addCatalog(...) is still used somewhere in the > > rejected > > > > > > section > > > > > > > > > which > > > > > > > > > > > should be persistContext(...) to keep it consistent. > > > > > > > > > > > > > > > > > > > > > > Speaking of the conflict issues in the multi-instance > > > > > scenarios, > > > > > > I > > > > > > > am > > > > > > > > > not > > > > > > > > > > > sure if this is the intended behaviour. If Map<String, > > > > Catalog> > > > > > > > > > catalogs > > > > > > > > > > is > > > > > > > > > > > used as a cache, it should be invalid, once the related > > > > catalog > > > > > > has > > > > > > > > > been > > > > > > > > > > > removed from the CatalogStore by another instance. Did > I > > > miss > > > > > > > > > something? > > > > > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > > > > > Jing > > > > > > > > > > > > > > > > > > > > > > On Thu, Apr 13, 2023 at 4:40 PM Feng Jin < > > > > > jinfeng1...@gmail.com> > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > Hi Jing,Shammon > > > > > > > > > > > > Thanks for your reply. > > > > > > > > > > > > > > > > > > > > > > > > @Jing > > > > > > > > > > > > > > > > > > > > > > > > > How about persistCatalog()? > > > > > > > > > > > > I think this is a good function name, I have updated > it > > > in > > > > > the > > > > > > > > > > > > documentation. > > > > > > > > > > > > > > > > > > > > > > > > > Some common cache features should be implemented > > > > > > > > > > > > Thank you for the suggestion. If alternative 1 turns > > out > > > to > > > > > be > > > > > > > more > > > > > > > > > > > > appropriate later, I will improve this part of the > > > content. > > > > > > > > > > > > > > > > > > > > > > > > > As the above discussion moves forward, the option 2 > > > > > solution > > > > > > > > looks > > > > > > > > > > more > > > > > > > > > > > > like a replacement of option 1 > > > > > > > > > > > > Yes, after discussing with Shammon offline, we think > > that > > > > > > > solution > > > > > > > > 2 > > > > > > > > > > > might > > > > > > > > > > > > be more suitable and also avoid any inconsistency > > issues. > > > > > > > > > > > > > > > > > > > > > > > > > There are some inconsistent descriptions in the > > > content. > > > > > > Would > > > > > > > > you > > > > > > > > > > > like > > > > > > > > > > > > to clean them up? > > > > > > > > > > > > I will do my best to improve the document and > > appreciate > > > > your > > > > > > > > > > > suggestions. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > @Shammon > > > > > > > > > > > > > can you put the unselected option in `Rejected > > > > > Alternatives` > > > > > > > > > > > > Sure, I have moved it to the `Rejected Alternatives`. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Best > > > > > > > > > > > > Feng > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Apr 13, 2023 at 8:52 AM Shammon FY < > > > > > zjur...@gmail.com> > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > Hi Feng > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks for your update. > > > > > > > > > > > > > > > > > > > > > > > > > > I found there are two options in `Proposed > Changes`, > > > can > > > > > you > > > > > > > put > > > > > > > > > the > > > > > > > > > > > > > unselected option in `Rejected Alternatives`? I > think > > > > this > > > > > > may > > > > > > > > help > > > > > > > > > > us > > > > > > > > > > > > > better understand your proposal > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > > > > > Shammon FY > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Apr 13, 2023 at 4:49 AM Jing Ge > > > > > > > > <j...@ververica.com.invalid > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Feng, > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks for raising this FLIP. I am still confused > > > after > > > > > > > > > completely > > > > > > > > > > > > > reading > > > > > > > > > > > > > > the thread with following questions: > > > > > > > > > > > > > > > > > > > > > > > > > > > > 1. Naming confusion - registerCatalog() and > > > > addCatalog() > > > > > > have > > > > > > > > no > > > > > > > > > > big > > > > > > > > > > > > > > difference based on their names. One of them is > > > > > responsible > > > > > > > for > > > > > > > > > > data > > > > > > > > > > > > > > persistence. How about persistCatalog()? > > > > > > > > > > > > > > 2. As you mentioned that Map<String, Catalog> > > > catalogs > > > > is > > > > > > > used > > > > > > > > > as a > > > > > > > > > > > > cache > > > > > > > > > > > > > > and catalogStore is used for data persistence. I > > > would > > > > > > > suggest > > > > > > > > > > > > describing > > > > > > > > > > > > > > their purpose conceptually and clearly in the > FLIP. > > > > Some > > > > > > > common > > > > > > > > > > cache > > > > > > > > > > > > > > features should be implemented, i.e. data in the > > > cache > > > > > and > > > > > > > the > > > > > > > > > > store > > > > > > > > > > > > > should > > > > > > > > > > > > > > be consistent. Same Catalog instance should be > > found > > > in > > > > > the > > > > > > > > store > > > > > > > > > > and > > > > > > > > > > > > in > > > > > > > > > > > > > > the cache(either it has been initialized or it > will > > > be > > > > > lazy > > > > > > > > > > > > initialized) > > > > > > > > > > > > > > for the same catalog name. The consistency will > be > > > > taken > > > > > > care > > > > > > > > of > > > > > > > > > > > while > > > > > > > > > > > > > > updating the catalog. > > > > > > > > > > > > > > 3. As the above discussion moves forward, the > > option > > > 2 > > > > > > > solution > > > > > > > > > > looks > > > > > > > > > > > > > more > > > > > > > > > > > > > > like a replacement of option 1, because, afaiu, > > > issues > > > > > > > > mentioned > > > > > > > > > > > > > > previously with option 1 are not solved yet. Do > you > > > > still > > > > > > > want > > > > > > > > to > > > > > > > > > > > > propose > > > > > > > > > > > > > > both options and ask for suggestions for both of > > > them? > > > > > > > > > > > > > > 4. After you updated the FLIP, there are some > > > > > inconsistent > > > > > > > > > > > descriptions > > > > > > > > > > > > > in > > > > > > > > > > > > > > the content. Would you like to clean them up? > > > Thanks! > > > > > > > > > > > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > > > > > > > > Jing > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Fri, Apr 7, 2023 at 9:24 AM Feng Jin < > > > > > > > jinfeng1...@gmail.com > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > hi Shammon > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thank you for your response, and I completely > > agree > > > > > with > > > > > > > your > > > > > > > > > > point > > > > > > > > > > > > of > > > > > > > > > > > > > > > view. > > > > > > > > > > > > > > > Initially, I may have over complicated the > whole > > > > issue. > > > > > > > First > > > > > > > > > and > > > > > > > > > > > > > > foremost, > > > > > > > > > > > > > > > we need to consider the persistence of the > > > Catalog's > > > > > > > > > > Configuration. > > > > > > > > > > > > > > > If we only need to provide persistence for > > Catalog > > > > > > > > > Configuration, > > > > > > > > > > > we > > > > > > > > > > > > > can > > > > > > > > > > > > > > > add a toConfiguration method to the Catalog > > > > interface. > > > > > > > > > > > > > > > This method can convert a Catalog instance to a > > > > > > Map<String, > > > > > > > > > > String> > > > > > > > > > > > > > > > properties, and the default implementation will > > > throw > > > > > an > > > > > > > > > > exception. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > public interface Catalog { > > > > > > > > > > > > > > > /** > > > > > > > > > > > > > > > * Returns a map containing the properties of > > the > > > > > > catalog > > > > > > > > > > object. > > > > > > > > > > > > > > > * > > > > > > > > > > > > > > > * @return a map containing the properties of > > the > > > > > > catalog > > > > > > > > > > object > > > > > > > > > > > > > > > * @throws UnsupportedOperationException if > the > > > > > > > > implementing > > > > > > > > > > > class > > > > > > > > > > > > > does > > > > > > > > > > > > > > > not override > > > > > > > > > > > > > > > * the default implementation of this method > > > > > > > > > > > > > > > */ > > > > > > > > > > > > > > > default Map<String, String> toProperties() { > > > > > > > > > > > > > > > throw new > > UnsupportedOperationException("Please > > > > > > > implement > > > > > > > > > > > > > > toProperties > > > > > > > > > > > > > > > for this catalog"); > > > > > > > > > > > > > > > } > > > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The specific process is as follows: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 1. If the user has configured a CatalogStore, > > the > > > > > > > > > > > toConfiguration() > > > > > > > > > > > > > > method > > > > > > > > > > > > > > > will be called when registering a Catalog > > instance > > > > with > > > > > > > > > > > > > > > registerCatalog(String catalogName, Catalog > > > catalog). > > > > > The > > > > > > > > > Catalog > > > > > > > > > > > > > > instance > > > > > > > > > > > > > > > will be converted to a Map<String, String> > > > properties > > > > > and > > > > > > > > saved > > > > > > > > > > to > > > > > > > > > > > > the > > > > > > > > > > > > > > > CatalogStore. If some Catalog instances do not > > > > > implement > > > > > > > this > > > > > > > > > > > method, > > > > > > > > > > > > > an > > > > > > > > > > > > > > > exception will be thrown. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2. If the user does not use a CatalogStore, > the > > > > > > > > > > toConfiguration() > > > > > > > > > > > > > method > > > > > > > > > > > > > > > will not be called, ensuring consistency with > the > > > > > > original > > > > > > > > > > process. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 3. Saving both the Map<String, Catalog> > catalogs > > > and > > > > > the > > > > > > > > > > > CatalogStore > > > > > > > > > > > > > at > > > > > > > > > > > > > > > the same time can also avoid conflicts > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > For lazy initialization: > > > > > > > > > > > > > > > we can start from the Catalog itself and > provide > > a > > > > > > > dedicated > > > > > > > > > > > Catalog > > > > > > > > > > > > > > > implementation for lazy loading, such as > > > > > > > ConfigurationCatalog > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > public class ConfigurationCatalog implements > > > Catalog > > > > { > > > > > > > > > > > > > > > ConfigurationCatalog(Map<String, String> > > > properties) > > > > { > > > > > > > > > > > > > > > } > > > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I have added this design to the FLIP. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > > > > > > > Feng > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Apr 6, 2023 at 10:03 AM Shammon FY < > > > > > > > > zjur...@gmail.com> > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Feng > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks for your answer. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I have no questions about the functionality > of > > > > > > > > > `CatalogStore`, > > > > > > > > > > > but > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > different operations of multiple > > > `registerCatalog` > > > > or > > > > > > > > > > > > `storeCatalog` > > > > > > > > > > > > > in > > > > > > > > > > > > > > > > `CatalogManager`. The implementation in Trino > > is > > > > also > > > > > > the > > > > > > > > > same, > > > > > > > > > > > the > > > > > > > > > > > > > > > > `CatalogManager` in trino only has one > > > > > `createCatalog`, > > > > > > > > which > > > > > > > > > > > will > > > > > > > > > > > > > save > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > catalog to memory and `CatalogStore`. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think we don't need to add another > > > > > `registerCatalog` > > > > > > or > > > > > > > > > > > > > `addCatalog` > > > > > > > > > > > > > > to > > > > > > > > > > > > > > > > save a catalog in `Map<String, Catalog> > > catalogs` > > > > or > > > > > > > > > > > `CatalogStore > > > > > > > > > > > > > > > > catalogStore`. As you mentioned before, the > > > > > > `Map<String, > > > > > > > > > > > Catalog> > > > > > > > > > > > > > > > > catalogs` is a cache in `CatalogManager`. How > > > about > > > > > > > saving > > > > > > > > > the > > > > > > > > > > > > > catalog > > > > > > > > > > > > > > in > > > > > > > > > > > > > > > > `Map<String, Catalog> catalogs` and > > `CatalogStore > > > > > > > > > catalogStore` > > > > > > > > > > > > > > together > > > > > > > > > > > > > > > > when it is registered or added in > > > `CatalogManager`? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > > > > > > > > Shammon FY > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Apr 4, 2023 at 5:22 PM Feng Jin < > > > > > > > > > jinfeng1...@gmail.com > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thank you for your reply. I am very sorry > for > > > the > > > > > > > > > > > > misunderstanding > > > > > > > > > > > > > > > caused > > > > > > > > > > > > > > > > > by my deviation from the original > discussion. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > @Shammon > > > > > > > > > > > > > > > > > > I found there is a pre-discussion [1] for > > > this > > > > > FLIP > > > > > > > > > > > > > > > > > Yes, there was indeed such a discussion > > before. > > > > > > > However, > > > > > > > > > in > > > > > > > > > > > > > > designing > > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > whole solution, I found that the logic of > > > > > > > CatalogManager > > > > > > > > > > itself > > > > > > > > > > > > > > doesn't > > > > > > > > > > > > > > > > > need to change much. *We cannot only > persist > > > > > Catalog > > > > > > > > > > instances > > > > > > > > > > > > > > > > themselves*, > > > > > > > > > > > > > > > > > so exposing only registerCatalog(String > > > > > catalogName, > > > > > > > > > Catalog > > > > > > > > > > > > > catalog) > > > > > > > > > > > > > > > > might > > > > > > > > > > > > > > > > > not be enough to save Catalogs, because in > > the > > > > end > > > > > we > > > > > > > > still > > > > > > > > > > > need > > > > > > > > > > > > to > > > > > > > > > > > > > > > save > > > > > > > > > > > > > > > > > the configurations corresponding to the > > Catalog > > > > > > > > instances. > > > > > > > > > > > > > > Therefore, > > > > > > > > > > > > > > > I > > > > > > > > > > > > > > > > > decided to introduce the CatalogStore > > interface > > > > for > > > > > > > > > > > configuration > > > > > > > > > > > > > > > > > persistence. Regarding this part, I also > took > > > > > > > inspiration > > > > > > > > > > from > > > > > > > > > > > > > > Trino's > > > > > > > > > > > > > > > > > implementation[1]. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > @yuxia > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 1: The mechanism of handling catalog with > > the > > > > > same > > > > > > > name > > > > > > > > > > > looks a > > > > > > > > > > > > > > > little > > > > > > > > > > > > > > > > of > > > > > > > > > > > > > > > > > complex to me. > > > > > > > > > > > > > > > > > Thank you for the suggestion. I will > provide > > a > > > > > > detailed > > > > > > > > > > > > description > > > > > > > > > > > > > > and > > > > > > > > > > > > > > > > > code examples for this part, and add it to > > the > > > > > FLIP. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2: TBH, the method name `addCatalog` > still > > > > looks > > > > > > > > confused > > > > > > > > > > to > > > > > > > > > > > > me. > > > > > > > > > > > > > > > IIUC, > > > > > > > > > > > > > > > > > this method is for storing catalog to > > > > CatalogStore, > > > > > > how > > > > > > > > > about > > > > > > > > > > > > > > renaming > > > > > > > > > > > > > > > it > > > > > > > > > > > > > > > > > to `storeCatalog`? It's very personal > > opinion, > > > > you > > > > > > can > > > > > > > > > decide > > > > > > > > > > > to > > > > > > > > > > > > > take > > > > > > > > > > > > > > > it > > > > > > > > > > > > > > > > or > > > > > > > > > > > > > > > > > not by your self. > > > > > > > > > > > > > > > > > StoreCatalog looks more intuitive to me. I > > > don't > > > > > see > > > > > > > any > > > > > > > > > > > problem > > > > > > > > > > > > > with > > > > > > > > > > > > > > > it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 3.For CREATE CATALOG statement, which > > method > > > > will > > > > > > be > > > > > > > > > > called? > > > > > > > > > > > > > > > > > `registerCatalog` or `addCatalog`? I'm > > > wondering > > > > > > > whether > > > > > > > > > user > > > > > > > > > > > can > > > > > > > > > > > > > > add a > > > > > > > > > > > > > > > > > catalog to store with SQL stement. > > > > > > > > > > > > > > > > > For CREATE CATALOG, my original design was > to > > > add > > > > > it > > > > > > > > > directly > > > > > > > > > > > to > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > CatalogStore, but this would disrupt the > > > existing > > > > > > > logic. > > > > > > > > > > > > > Therefore, I > > > > > > > > > > > > > > > > think > > > > > > > > > > > > > > > > > we can do both: save the configuration to > the > > > > > > > > CatalogStore > > > > > > > > > > and > > > > > > > > > > > > > > > initialize > > > > > > > > > > > > > > > > > the Catalog instance at the same time > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 3. Is it really neccessary to provide a > > > default > > > > > > > > > > implmentation > > > > > > > > > > > > for > > > > > > > > > > > > > > > > > interface `CatalogStoreFactory`? > > > > > > > > > > > > > > > > > I think it is necessary, otherwise we would > > > need > > > > to > > > > > > > > > introduce > > > > > > > > > > > an > > > > > > > > > > > > > > > > additional > > > > > > > > > > > > > > > > > Map to store the configuration for lazy > > > loading. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 4: About asynchronous registration for > > > catalog. > > > > > > > > > > > > > > > > > I don't think registerCatalog(String > > > catalogName, > > > > > > > Catalog > > > > > > > > > > > > catalog) > > > > > > > > > > > > > > can > > > > > > > > > > > > > > > be > > > > > > > > > > > > > > > > > made into an asynchronous interface because > > > > Catalog > > > > > > is > > > > > > > > > > already > > > > > > > > > > > an > > > > > > > > > > > > > > > > instance. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > It looks more like lazy initialization > for > > > > > catalog > > > > > > > than > > > > > > > > > > > > > > asynchronous > > > > > > > > > > > > > > > > > registration, right? > > > > > > > > > > > > > > > > > Indeed, my description was inaccurate. It > > > should > > > > be > > > > > > > lazy > > > > > > > > > > > > > registration > > > > > > > > > > > > > > > > > instead of asynchronous registration. I > have > > > > > already > > > > > > > > > updated > > > > > > > > > > > the > > > > > > > > > > > > > > title > > > > > > > > > > > > > > > of > > > > > > > > > > > > > > > > > the FLIP. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > [1]. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/trinodb/trino/blob/master/core/trino-main/src/main/java/io/trino/connector/CatalogStore.java > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Apr 4, 2023 at 4:27 PM Shammon FY < > > > > > > > > > zjur...@gmail.com > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Feng > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think if there is a `registerCatalog` > > > method > > > > in > > > > > > > > > > > > > `CatalogManager`, > > > > > > > > > > > > > > > it > > > > > > > > > > > > > > > > > will > > > > > > > > > > > > > > > > > > confuse users whether a method named > > > > `addCatalog` > > > > > > or > > > > > > > > > > > > > `storeCatalog` > > > > > > > > > > > > > > > is > > > > > > > > > > > > > > > > > > added. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > And as you mentioned, the memory catalog > > is a > > > > > > > `cache`, > > > > > > > > I > > > > > > > > > > > think > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > concept > > > > > > > > > > > > > > > > > > of `cache` should not be exposed to > users. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I found there is a pre-discussion [1] for > > > this > > > > > > FLIP. > > > > > > > > > Please > > > > > > > > > > > > > correct > > > > > > > > > > > > > > > me > > > > > > > > > > > > > > > > if > > > > > > > > > > > > > > > > > > I'm wrong, IIUC, the conclusion of that > > > > > discussion > > > > > > is > > > > > > > > to > > > > > > > > > > use > > > > > > > > > > > > > > > > > > `CatalogManager` as an interface and > > > implement > > > > it > > > > > > for > > > > > > > > > > > different > > > > > > > > > > > > > > > stores > > > > > > > > > > > > > > > > > such > > > > > > > > > > > > > > > > > > as memory, file and external system. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think there is a gap between the > current > > > FLIP > > > > > > > design > > > > > > > > > and > > > > > > > > > > > that > > > > > > > > > > > > > > > > > conclusion. > > > > > > > > > > > > > > > > > > What about the proposal of the discussion > > in > > > > > thread > > > > > > > > [1] ? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > [1] > > > > > > > > > > > > > > > > > > > > > > > > https://lists.apache.org/thread/9bnjblgd9wvrl75lkm84oo654c4lqv70 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > > > > > > > > > > Shammon FY > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Apr 4, 2023 at 3:41 PM yuxia < > > > > > > > > > > > > > luoyu...@alumni.sjtu.edu.cn> > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks Feng for driving this FLIP. > > > > > > > > > > > > > > > > > > > I have few comments: > > > > > > > > > > > > > > > > > > > 1: The mechanism of handling catalog > with > > > the > > > > > > same > > > > > > > > name > > > > > > > > > > > > looks a > > > > > > > > > > > > > > > > little > > > > > > > > > > > > > > > > > of > > > > > > > > > > > > > > > > > > > complex to me. I think it'll be better > to > > > > > explain > > > > > > > it > > > > > > > > in > > > > > > > > > > the > > > > > > > > > > > > > java > > > > > > > > > > > > > > > doc > > > > > > > > > > > > > > > > of > > > > > > > > > > > > > > > > > > > these methods and give a brief example > in > > > > this > > > > > > > FLIP. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2: TBH, the method name `addCatalog` > > still > > > > > looks > > > > > > > > > confused > > > > > > > > > > > to > > > > > > > > > > > > > me. > > > > > > > > > > > > > > > > IIUC, > > > > > > > > > > > > > > > > > > > this method is for storing catalog to > > > > > > CatalogStore, > > > > > > > > how > > > > > > > > > > > about > > > > > > > > > > > > > > > > renaming > > > > > > > > > > > > > > > > > it > > > > > > > > > > > > > > > > > > > to `storeCatalog`? It's very personal > > > > opinion, > > > > > > you > > > > > > > > can > > > > > > > > > > > decide > > > > > > > > > > > > > to > > > > > > > > > > > > > > > take > > > > > > > > > > > > > > > > > it > > > > > > > > > > > > > > > > > > or > > > > > > > > > > > > > > > > > > > not by your self. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 3: For CREATE CATALOG statement, which > > > method > > > > > > will > > > > > > > be > > > > > > > > > > > called? > > > > > > > > > > > > > > > > > > > `registerCatalog` or `addCatalog`? I'm > > > > > wondering > > > > > > > > > whether > > > > > > > > > > > user > > > > > > > > > > > > > can > > > > > > > > > > > > > > > > add a > > > > > > > > > > > > > > > > > > > catalog to store with SQL stement. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 3: Is it really neccessary to provide a > > > > default > > > > > > > > > > > implmentation > > > > > > > > > > > > > for > > > > > > > > > > > > > > > > > > > interface `CatalogStoreFactory`? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 4: About asynchronous registration for > > > > catalog. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > When creating a catalog with CREATE > > > > CATALOG, > > > > > > the > > > > > > > > > > > > asynchronous > > > > > > > > > > > > > > > > > > > registration method is used by default. > > > > > > > > > > > > > > > > > > > If asynchronous registration is the > > default > > > > > > > behavior, > > > > > > > > > it > > > > > > > > > > > > there > > > > > > > > > > > > > > any > > > > > > > > > > > > > > > > way > > > > > > > > > > > > > > > > > > > that user can switch to synchronous > > > > > registration > > > > > > > just > > > > > > > > > > like > > > > > > > > > > > > > > before? > > > > > > > > > > > > > > > > > > > Will both method `addCatalog` and > > > > > > `registerCatalog` > > > > > > > > be > > > > > > > > > > > > > > asynchronous > > > > > > > > > > > > > > > > > > > registration? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > IIUC, in asynchronous registration, it > > may > > > > well > > > > > > > that > > > > > > > > > > CREATE > > > > > > > > > > > > > > CATALOG > > > > > > > > > > > > > > > > > > > executes successfully, but then the > > > following > > > > > > > CREATE > > > > > > > > > > TABLE > > > > > > > > > > > > > > > statement > > > > > > > > > > > > > > > > > will > > > > > > > > > > > > > > > > > > > fail for the catalog fail to open. > > > > > > > > > > > > > > > > > > > I think it's a break change which > should > > be > > > > > > > > highlighted > > > > > > > > > > in > > > > > > > > > > > > this > > > > > > > > > > > > > > > FLIP, > > > > > > > > > > > > > > > > > may > > > > > > > > > > > > > > > > > > > be in compatibility part. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > BTW, by saying asynchronous > > registration, I > > > > > would > > > > > > > > like > > > > > > > > > to > > > > > > > > > > > > > expect > > > > > > > > > > > > > > > > there > > > > > > > > > > > > > > > > > > > will be an executor to open or register > > > > catalog > > > > > > in > > > > > > > > the > > > > > > > > > > > > > > background, > > > > > > > > > > > > > > > > but > > > > > > > > > > > > > > > > > > from > > > > > > > > > > > > > > > > > > > your previous comments, > > > > > > > > > > > > > > > > > > > "the Catalog instance will be > initialized > > > if > > > > it > > > > > > has > > > > > > > > not > > > > > > > > > > > been > > > > > > > > > > > > > > > > > initialized > > > > > > > > > > > > > > > > > > > yet. If the initialization process > fails, > > > > these > > > > > > > > > > statements > > > > > > > > > > > > will > > > > > > > > > > > > > > not > > > > > > > > > > > > > > > > be > > > > > > > > > > > > > > > > > > > executed successfully." > > > > > > > > > > > > > > > > > > > It looks more like lazy initialization > > for > > > > > > catalog > > > > > > > > than > > > > > > > > > > > > > > > asynchronous > > > > > > > > > > > > > > > > > > > registration, right? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > > > > > > > > > > > > > Yuxia > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ----- 原始邮件 ----- > > > > > > > > > > > > > > > > > > > 发件人: "Feng Jin" <jinfeng1...@gmail.com > > > > > > > > > > > > > > > > > > > > > 收件人: "dev" <dev@flink.apache.org> > > > > > > > > > > > > > > > > > > > 发送时间: 星期一, 2023年 4 月 03日 下午 3:27:45 > > > > > > > > > > > > > > > > > > > 主题: Re: [DISCUSS] FLIP 295: Support > > > > persistence > > > > > > of > > > > > > > > > > Catalog > > > > > > > > > > > > > > > > > configuration > > > > > > > > > > > > > > > > > > > and asynchronous registration > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi everyone, Thank you all for your > > > interest > > > > in > > > > > > > this > > > > > > > > > > > DISCUSS. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > @Shammon > > > > > > > > > > > > > > > > > > > > How to handle a catalog with the same > > > name > > > > > that > > > > > > > > > exists > > > > > > > > > > > for > > > > > > > > > > > > > both > > > > > > > > > > > > > > > of > > > > > > > > > > > > > > > > > > them? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I believe this is a crucial point. > Based > > on > > > > my > > > > > > > > current > > > > > > > > > > > > personal > > > > > > > > > > > > > > > > > > > understanding, the Map<String, Catalog> > > > > > catalogs > > > > > > > will > > > > > > > > > > serve > > > > > > > > > > > > as > > > > > > > > > > > > > a > > > > > > > > > > > > > > > > cache > > > > > > > > > > > > > > > > > > > for instantiated catalogs and have the > > > > highest > > > > > > > > > priority. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > There are three methods that can affect > > the > > > > > > > > Map<String, > > > > > > > > > > > > > Catalog> > > > > > > > > > > > > > > > > > > catalogs: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 1. registerCatalog(String catalogName, > > > > Catalog > > > > > > > > catalog) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This method puts the catalog instance > > into > > > > the > > > > > > > > > > Map<String, > > > > > > > > > > > > > > Catalog> > > > > > > > > > > > > > > > > > > > catalogs. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2. unregisterCatalog(String > catalogName) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This method removes the catalog > instance > > > > > > > > corresponding > > > > > > > > > to > > > > > > > > > > > > > > > catalogName > > > > > > > > > > > > > > > > > > > from the Map<String, Catalog> catalogs. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 3. getCatalog(String catalogName) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This method first retrieves the > > > corresponding > > > > > > > catalog > > > > > > > > > > > > instance > > > > > > > > > > > > > > from > > > > > > > > > > > > > > > > > > > the Map<String, Catalog> catalogs. If > the > > > > > catalog > > > > > > > > does > > > > > > > > > > not > > > > > > > > > > > > > exist, > > > > > > > > > > > > > > > it > > > > > > > > > > > > > > > > > > > retrieves the corresponding > configuration > > > > from > > > > > > the > > > > > > > > > > > > > CatalogStore, > > > > > > > > > > > > > > > > > > > initializes it, and puts the > initialized > > > > > Catalog > > > > > > > > > instance > > > > > > > > > > > > into > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > Map<String, Catalog> catalogs. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The following two methods only modify > the > > > > > > > > configuration > > > > > > > > > > in > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > CatalogStore: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 1. addCatalog(String catalogName, > > > Map<String, > > > > > > > String> > > > > > > > > > > > > > properties) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This method saves the properties to the > > > > > > > catalogStore > > > > > > > > > and > > > > > > > > > > > > checks > > > > > > > > > > > > > > > > > > > whether there is a catalogName with the > > > same > > > > > > name. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2. removeCatalog(String catalogName) > > > > > > > > > > > > > > > > > > > This method removes the specified > > > > configuration > > > > > > of > > > > > > > > the > > > > > > > > > > > > > specified > > > > > > > > > > > > > > > > > > > catalogName in the catalogStore. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The following are possible conflict > > > > scenarios: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 1. When the corresponding catalogName > > > already > > > > > > > exists > > > > > > > > in > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > CatalogStore but not in the Map<String, > > > > > Catalog>, > > > > > > > the > > > > > > > > > > > > > > > > > > > registerCatalog(String catalogName, > > Catalog > > > > > > > catalog) > > > > > > > > > > method > > > > > > > > > > > > can > > > > > > > > > > > > > > > > > > > succeed and be directly saved to the > > > > > Map<String, > > > > > > > > > Catalog> > > > > > > > > > > > > > > catalogs. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2. When the corresponding catalogName > > > already > > > > > > > exists > > > > > > > > in > > > > > > > > > > > both > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > CatalogStore and the Map<String, > > Catalog>, > > > > the > > > > > > > > > > > > > > > registerCatalog(String > > > > > > > > > > > > > > > > > > > catalogName, Catalog catalog) method > will > > > > fail. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 3. When the corresponding catalogName > > > already > > > > > > > exists > > > > > > > > in > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > Map<String, Catalog>, the > > addCatalog(String > > > > > > > > > catalogName, > > > > > > > > > > > > > > > Map<String, > > > > > > > > > > > > > > > > > > > String> properties) method can directly > > > save > > > > > the > > > > > > > > > > properties > > > > > > > > > > > > to > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > catalogStore, but the getCatalog(String > > > > > > > catalogName) > > > > > > > > > > method > > > > > > > > > > > > > will > > > > > > > > > > > > > > > not > > > > > > > > > > > > > > > > > > > use the new properties for > initialization > > > > > because > > > > > > > the > > > > > > > > > > > > > > corresponding > > > > > > > > > > > > > > > > > > > catalog instance already exists in > > catalogs > > > > and > > > > > > > will > > > > > > > > be > > > > > > > > > > > > > > > prioritized. > > > > > > > > > > > > > > > > > > > Therefore, using the > > > unregisterCatalog(String > > > > > > > > > > catalogName) > > > > > > > > > > > > > method > > > > > > > > > > > > > > > to > > > > > > > > > > > > > > > > > > > remove the instance corresponding to > the > > > > > original > > > > > > > > > > > catalogName > > > > > > > > > > > > > is > > > > > > > > > > > > > > > > > > > necessary. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think it will confuse users that > > > > > > > > > > > `registerCatalog(String > > > > > > > > > > > > > > > > > > > catalogName,Catalog catalog)` in the > > > > > `Map<String, > > > > > > > > > > Catalog> > > > > > > > > > > > > > > > catalogs` > > > > > > > > > > > > > > > > > and > > > > > > > > > > > > > > > > > > > `registerCatalog(String catalogName, > > > > > Map<String, > > > > > > > > > String> > > > > > > > > > > > > > > > properties) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This could potentially lead to > > confusion. I > > > > > > suggest > > > > > > > > > > > changing > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > method name, perhaps to > addCatalog(String > > > > > > > > catalogName, > > > > > > > > > > > > > > Map<String, > > > > > > > > > > > > > > > > > > > String> properties), as previously > > > mentioned > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > @Hang > > > > > > > > > > > > > > > > > > > > add `registerCatalog(String > > > > > catalogName,Catalog > > > > > > > > > > catalog, > > > > > > > > > > > > > > > > > > > boolean lazyInit)` method > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Since a catalog is already an instance, > > > > adding > > > > > > the > > > > > > > > > > > "lazyInit" > > > > > > > > > > > > > > > > > > > parameter to the registerCatalog(String > > > > > > > catalogName, > > > > > > > > > > > Catalog > > > > > > > > > > > > > > > catalog) > > > > > > > > > > > > > > > > > > > method may not necessarily result in > lazy > > > > > > > > > initialization. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Do we need to think about encryption > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think encryption is necessary, but > > > perhaps > > > > > the > > > > > > > > > > encryption > > > > > > > > > > > > > logic > > > > > > > > > > > > > > > > > > > should be implemented in the > CatalogStore > > > > when > > > > > > the > > > > > > > > data > > > > > > > > > > is > > > > > > > > > > > > > > actually > > > > > > > > > > > > > > > > > > > saved. For instance, the > FileCatalogStore > > > > could > > > > > > > > encrypt > > > > > > > > > > the > > > > > > > > > > > > > data > > > > > > > > > > > > > > > when > > > > > > > > > > > > > > > > > > > it is saved to a file, while the > > > > > > MemoryCatalogStore > > > > > > > > > does > > > > > > > > > > > not > > > > > > > > > > > > > > > require > > > > > > > > > > > > > > > > > > > encryption. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Do we really need the > > > > `MemoryCatalogStore`? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think it is necessary to have a > > > > > > > MemoryCatalogStore > > > > > > > > as > > > > > > > > > > the > > > > > > > > > > > > > > default > > > > > > > > > > > > > > > > > > > implementation, which saves Catalog > > > > > > configurations > > > > > > > in > > > > > > > > > > > memory. > > > > > > > > > > > > > > > > > > > Otherwise, if we want to implement > > > > asynchronous > > > > > > > > loading > > > > > > > > > > of > > > > > > > > > > > > > > Catalog, > > > > > > > > > > > > > > > > we > > > > > > > > > > > > > > > > > > > would need to introduce additional > cache. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > @Xianxun > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > What if asynchronous registration > > failed? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This is also a critical concern. When > > > > > executing > > > > > > > DDL, > > > > > > > > > > DQL, > > > > > > > > > > > or > > > > > > > > > > > > > DML > > > > > > > > > > > > > > > > > > > statements that reference a specific > > > Catalog, > > > > > the > > > > > > > > > Catalog > > > > > > > > > > > > > > instance > > > > > > > > > > > > > > > > > > > will be initialized if it has not been > > > > > > initialized > > > > > > > > yet. > > > > > > > > > > If > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > initialization process fails, these > > > > statements > > > > > > will > > > > > > > > not > > > > > > > > > > be > > > > > > > > > > > > > > executed > > > > > > > > > > > > > > > > > > > successfully. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > nt. The Map<String, Catalog> catalogs > > and > > > > > > > > > CatalogStore > > > > > > > > > > > > > > > catalogstore > > > > > > > > > > > > > > > > > in > > > > > > > > > > > > > > > > > > > the CatalogManager all have the same > > > catalog > > > > > > name, > > > > > > > > but > > > > > > > > > > > > > > correponding > > > > > > > > > > > > > > > > to > > > > > > > > > > > > > > > > > > > different catalog instances. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This issue can be resolved by referring > > to > > > > the > > > > > > > > previous > > > > > > > > > > > > > > responses. > > > > > > > > > > > > > > > > The > > > > > > > > > > > > > > > > > > > key principle is that the Catalog that > > has > > > > been > > > > > > > most > > > > > > > > > > > recently > > > > > > > > > > > > > > used > > > > > > > > > > > > > > > > > > > will be given priority for subsequent > > use. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Sun, Apr 2, 2023 at 10:58 PM Xianxun > > Ye > > > < > > > > > > > > > > > > > > > yesorno828...@gmail.com> > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Feng, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks for driving this Flip, I do > > > believe > > > > > this > > > > > > > > Flip > > > > > > > > > > > could > > > > > > > > > > > > be > > > > > > > > > > > > > > > > helpful > > > > > > > > > > > > > > > > > > > for users. The firm I work for also > > > manages a > > > > > lot > > > > > > > of > > > > > > > > > > > > catalogs, > > > > > > > > > > > > > > and > > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > submission of tasks becomes slow > because > > of > > > > > > > loading a > > > > > > > > > > > number > > > > > > > > > > > > of > > > > > > > > > > > > > > > > > catalogs. > > > > > > > > > > > > > > > > > > > We obtain the catalogs in the user's > > Flink > > > > SQL > > > > > > > > through > > > > > > > > > > > > regular > > > > > > > > > > > > > > > > > expression > > > > > > > > > > > > > > > > > > > to avoid loading all catalogs to > improve > > > the > > > > > > speed > > > > > > > of > > > > > > > > > > task > > > > > > > > > > > > > > > > submission. > > > > > > > > > > > > > > > > > > > After reading flip-295, I have some > > > > questions: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 1. When creating a catalog with > CREATE > > > > > CATALOG, > > > > > > > the > > > > > > > > > > > > > > asynchronous > > > > > > > > > > > > > > > > > > > registration method is used by default. > > > What > > > > if > > > > > > > > > > > asynchronous > > > > > > > > > > > > > > > > > registration > > > > > > > > > > > > > > > > > > > failed? And how to implement > asynchronous > > > > > > > > registration? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2. I also have the same question with > > > > > Shammon’s > > > > > > > > first > > > > > > > > > > > > > comment. > > > > > > > > > > > > > > > The > > > > > > > > > > > > > > > > > > > Map<String, Catalog> catalogs and > > > > CatalogStore > > > > > > > > > > catalogstore > > > > > > > > > > > > in > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > CatalogManager all have the same > catalog > > > > name, > > > > > > but > > > > > > > > > > > > correponding > > > > > > > > > > > > > > to > > > > > > > > > > > > > > > > > > > different catalog instances. For > example: > > > > > > > catalog1-> > > > > > > > > > > > > > jdbcCatalog > > > > > > > > > > > > > > in > > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > catalogs, but catalog1 -> hiveCatalog > in > > > the > > > > > > > > > > catalogstore. > > > > > > > > > > > > > Would > > > > > > > > > > > > > > > this > > > > > > > > > > > > > > > > > > case > > > > > > > > > > > > > > > > > > > happen? Maybe we should define > `catalogs` > > > and > > > > > > > > > > > `catalogstore` > > > > > > > > > > > > > more > > > > > > > > > > > > > > > > > > clearly. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Xianxun > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2023年3月31日 11:13，Hang Ruan < > > > > > > > > ruanhang1...@gmail.com > > > > > > > > > > > > > > > > > > > > > 写道： > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > MemoryCatalogStore > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
