Hi jark, thanks for your suggestion. > 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.
Yes, We should initialize the CatalogStore when creating the TableEnv. Therefore, my current proposal is to add a method to configure the CatalogStore in EnvironmentSettings. // Initialize a catalog Store CatalogStore catalogStore = new FileCatalogStore(""); // set up the Table API final EnvironmentSettings settings = EnvironmentSettings.newInstance().inBatchMode() .withCatalogStore(catalogStore) .build(); final TableEnvironment tableEnv = TableEnvironment.create(settings); > 2. Why does the CatalogStoreFactory interface only have a default method, not an interface method? Sorry, While I did refer to the Catalog interface, I agree that as a new interface, the CatalogStoreFactory should not have a default method but an interface method. I have already modified the interface. > 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. Thanks for the suggestion, I have updated the FLIP [1]. [1]. https://cwiki.apache.org/confluence/display/FLINK/FLIP-295%3A+Support+lazy+initialization+of+catalogs+and+persistence+of+catalog+configurations Best, Feng On Thu, Jun 1, 2023 at 9:22 PM Jark Wu <imj...@gmail.com> wrote: > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >