You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "adutra (via GitHub)" <gi...@apache.org> on 2026/02/04 12:46:09 UTC
[PR] Remove "giant" constructors from Handlers and Adapters [polaris]
adutra opened a new pull request, #3669:
URL: https://github.com/apache/polaris/pull/3669
This PR attempts to reduce the "giant constructor" symptoms in catalog adapters and handlers.
It combines two techniques:
* The injectable runtime dependencies were grouped together in a "runtime" object, inspired by the "Parameter Object" pattern. These objects are `@PolarisImmutable` objects, and thus can leverage the generated builder to create instances without the need for big constructors.
* The runtime object is then produced by a new factory, which is a CDI request-scoped bean with direct field injection to avoid constructor injection.
<!--
๐ Describe what changes you're proposing, especially breaking or user-facing changes.
๐ See https://github.com/apache/polaris/blob/main/CONTRIBUTING.md for more.
-->
## Checklist
- [ ] ๐ก๏ธ Don't disclose security issues! (contact security@apache.org)
- [ ] ๐ Clearly explained why the changes are needed, or linked related issues: Fixes #
- [ ] ๐งช Added/updated tests with good coverage, or manually tested (and explained how)
- [ ] ๐ก Added comments for complex logic
- [ ] ๐งพ Updated `CHANGELOG.md` (if needed)
- [ ] ๐ Updated documentation in `site/content/in-dev/unreleased` (if needed)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove "giant" constructors from Handlers and Adapters [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #3669:
URL: https://github.com/apache/polaris/pull/3669#discussion_r2769058912
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -59,43 +52,25 @@ public abstract class CatalogHandler {
// Initialized in the authorize methods.
protected PolarisResolutionManifest resolutionManifest = null;
- protected final ResolutionManifestFactory resolutionManifestFactory;
Review Comment:
Done, cf. commit 99a05f3.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove "giant" constructors from Handlers and Adapters [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #3669:
URL: https://github.com/apache/polaris/pull/3669#discussion_r2769058912
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -59,43 +52,25 @@ public abstract class CatalogHandler {
// Initialized in the authorize methods.
protected PolarisResolutionManifest resolutionManifest = null;
- protected final ResolutionManifestFactory resolutionManifestFactory;
Review Comment:
Done, cf commit 03d15ea.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove "giant" constructors from Handlers and Adapters [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3669:
URL: https://github.com/apache/polaris/pull/3669#discussion_r2766638355
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -59,43 +52,25 @@ public abstract class CatalogHandler {
// Initialized in the authorize methods.
protected PolarisResolutionManifest resolutionManifest = null;
- protected final ResolutionManifestFactory resolutionManifestFactory;
Review Comment:
Just an idea: If we convert fields to abstract getters and make implementations of this class `@Immutable`, the `CatalogHandlerRuntime` object will probably become unnecessary and impl. instances will have direct builders... WDYT?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove "giant" constructors from Handlers and Adapters [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3669:
URL: https://github.com/apache/polaris/pull/3669#discussion_r2769887115
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -56,48 +55,36 @@
*/
public abstract class CatalogHandler {
- // Initialized in the authorize methods.
- protected PolarisResolutionManifest resolutionManifest = null;
+ public abstract String catalogName();
+
+ public abstract PolarisPrincipal polarisPrincipal();
- protected final ResolutionManifestFactory resolutionManifestFactory;
- protected final String catalogName;
- protected final PolarisAuthorizer authorizer;
- protected final PolarisCredentialManager credentialManager;
- protected final Instance<ExternalCatalogFactory> externalCatalogFactories;
-
- protected final PolarisDiagnostics diagnostics;
- protected final CallContext callContext;
- protected final RealmConfig realmConfig;
- protected final PolarisPrincipal polarisPrincipal;
-
- public CatalogHandler(
- PolarisDiagnostics diagnostics,
- CallContext callContext,
- ResolutionManifestFactory resolutionManifestFactory,
- PolarisPrincipal principal,
- String catalogName,
- PolarisAuthorizer authorizer,
- PolarisCredentialManager credentialManager,
- Instance<ExternalCatalogFactory> externalCatalogFactories) {
- this.diagnostics = diagnostics;
- this.callContext = callContext;
- this.realmConfig = callContext.getRealmConfig();
- this.resolutionManifestFactory = resolutionManifestFactory;
- this.catalogName = catalogName;
- this.polarisPrincipal = principal;
- this.authorizer = authorizer;
- this.credentialManager = credentialManager;
- this.externalCatalogFactories = externalCatalogFactories;
+ public abstract CallContext callContext();
+
+ @Value.Derived
+ public RealmConfig realmConfig() {
+ return callContext().getRealmConfig();
}
- protected PolarisCredentialManager getPolarisCredentialManager() {
- return credentialManager;
+ @Value.Derived
+ public RealmContext realmContext() {
+ return callContext().getRealmContext();
}
+ public abstract PolarisMetaStoreManager metaStoreManager();
+
+ public abstract ResolutionManifestFactory resolutionManifestFactory();
+
+ public abstract PolarisAuthorizer authorizer();
+
protected PolarisResolutionManifest newResolutionManifest() {
- return resolutionManifestFactory.createResolutionManifest(polarisPrincipal, catalogName);
+ return resolutionManifestFactory().createResolutionManifest(polarisPrincipal(), catalogName());
}
+ // Initialized in the authorize methods.
+ @SuppressWarnings("immutables:incompat")
+ protected PolarisResolutionManifest resolutionManifest = null;
Review Comment:
This might be fix/addressed if we go ahead with what @sungwy proposed for AuthZ refactoring plus my additional proposal for handling the "manifest" per API call: https://lists.apache.org/thread/srqp2jtts5438drzcvky47mdt8zs80wt
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove "giant" constructors from Handlers and Adapters [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #3669:
URL: https://github.com/apache/polaris/pull/3669#discussion_r2769052709
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -59,43 +52,25 @@ public abstract class CatalogHandler {
// Initialized in the authorize methods.
protected PolarisResolutionManifest resolutionManifest = null;
- protected final ResolutionManifestFactory resolutionManifestFactory;
Review Comment:
I had this approach initially but changed for the Parameter Object pattern, for a few reasons:
1. `CatalogHandler` and subclasses are inherently mutable. Fields like `resolutionManifest` are lazily evaluated, but are hard to migrate to a `@Value.Lazy` equivalent because they may be evaluated more than once.
2. Getters in `CatalogHandler` would have to be public, because it's in a different package from its subclasses, and thus the generated builders only compile if the getters are public.
That said, I don't mind showing my initial version, maybe it's acceptable indeed. The advantage being, that we could look into removing the mutable state little by little until the class hierarchy becomes fully immutable.
I will add a commit on top of this branch to show the approach.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove "giant" constructors from Handlers and Adapters [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #3669:
URL: https://github.com/apache/polaris/pull/3669#discussion_r2769058912
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -59,43 +52,25 @@ public abstract class CatalogHandler {
// Initialized in the authorize methods.
protected PolarisResolutionManifest resolutionManifest = null;
- protected final ResolutionManifestFactory resolutionManifestFactory;
Review Comment:
Done, cf. commit 63c3abd.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove "giant" constructors from Handlers and Adapters [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra merged PR #3669:
URL: https://github.com/apache/polaris/pull/3669
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org