lasdf1234 commented on PR #11043: URL: https://github.com/apache/gravitino/pull/11043#issuecomment-4428049058
> > I have concern about the code structure > > > > 1. The code structure refers to the project storage module. > > The storage module gave a complex abstraction to manage metadata objects and different relations. It used a heavy framework. Our plugin module should adopt lightweight implementation. > > I don't think we should use PO and mapper. > > 2. The code depends the module core > > It may be acceptable. But I hopes we should try to avoid depending it. > > This PR would still depend on core because its SQL provider factory and mapper registration mechanism already rely on shared relational infrastructure from 'core module'. > > > I have concern about the code structure > > > > 1. The code structure refers to the project storage module. > > The storage module gave a complex abstraction to manage metadata objects and different relations. It used a heavy framework. Our plugin module should adopt lightweight implementation. > > I don't think we should use PO and mapper. > > 2. The code depends the module core > > It may be acceptable. But I hopes we should try to avoid depending it. > > Here are the shared `core` capabilities still used by `idp-basic` beyond the `Idp*Meta` interfaces: > > Shared capability Location in `core` How `idp-basic` uses it Files > `JDBCBackendType` relational backend definition in `core` SQL provider factories use it to choose MySQL/H2/PostgreSQL providers `IdpUserMetaSQLProviderFactory.java:25,48`, `IdpGroupMetaSQLProviderFactory.java:24,47`, `IdpGroupUserRelSQLProviderFactory.java:25,49` > `SqlSessionFactoryHelper` `core/.../storage/relational/session/SqlSessionFactoryHelper.java` Reads MyBatis `databaseId`, then the factory picks the matching provider `IdpUserMetaSQLProviderFactory.java:30,42-46`, `IdpGroupMetaSQLProviderFactory.java:29,41-45`, `IdpGroupUserRelSQLProviderFactory.java:30,43-47` > `MapperPackageProvider` SPI `core/.../storage/relational/mapper/provider/MapperPackageProvider.java` `IdpBasicMapperPackageProvider` implements this SPI so the plugin’s mappers can be registered into MyBatis `plugins/idp-basic/.../provider/IdpBasicMapperPackageProvider.java:28-34` > This PR would still depend on core because its SQL provider factory and mapper registration mechanism already rely on shared relational infrastructure from core. cc @roryqi @jerryshao Was the idp-basic module implemented by itself? Is it better to rely on the core module? Which one is better? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
