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
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
