XN137 commented on PR #2540: URL: https://github.com/apache/polaris/pull/2540#issuecomment-3328488764
sorry i was OOO for a week. thank you for the feedback everyone! > IMHO, a "factory" makes more sense to be an ApplicationScoped bean, that once initialized, other methods/threads could use it to generate the real instance. i dont think there is a strict requirement to have factories be application scoped. a factory is just serving as an injectable decision point about how specific instances of an interface are supposed to be created (for a particular subset of the system). that doesnt immediately imply requirements on the lifetime scope of the factory or the produced objects. for example on `main` branch the `PolarisResolutionManifest` uses the `ResolverFactory` to produce a `primaryResolver` field immediately in the ctor and a "single-use" `passthroughResolver` on the fly in `getPassthroughResolvedPath`. generally `ResolverFactory` is only used by `PolarisResolutionManifest` and `IcebergCatalogAdapter` both of which afaict have a lifetime bound to a single request. this is indicated by both of them having a `CallContext` field, which gets initialized at construction time. > Making factories request-scoped seem to indicate that we do not need actually need these factories, and we can directly have request-scoped Resolver and ResolutionManfiest bean. Otherwise we are generating an additional dispensible factory object for every request. i would disagree with this because as shown in this PR both `ResolverFactory` and `ResolutionManifestFactory` cant be used without a `CallContext` which is only available in a request-scope. also, as pointed out above, the factory is also used on the fly to generate multiple instances inside a single object / the "same request". so while the factory currently is application-scoped it cant be used outside of a request-scope and this means all callers must pass all the required parameters around all the time, which this PR tries to avoid/clean up. > I think in general the factory design could allow us to add potential customization in the future and maybe some of them could be cached at realm-level instead of every request. correct me if i am wrong but i think even with making the factories request-scope (which matches their usage in the codebase) the implementation of that factory is still free to implement the object creation however it wants. so if something is supposed to be cached at realm-level the factory is still free to do that. (for example by injecting an application scoped bean that handles shared realm state). but this custom implementation decision does not need to be part of the factory usage in the codebase. a request-scoped factory can also return a singleton object if it really wants. -- 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]
