dennishuo commented on code in PR #414:
URL: https://github.com/apache/polaris/pull/414#discussion_r1825022161
##########
polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java:
##########
@@ -834,7 +834,10 @@ public LoadTableResponse loadTableWithAccessDelegation(
callContext.getPolarisCallContext(),
catalogEntity,
PolarisConfiguration.ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING)) {
- throw new ForbiddenException("Access Delegation is not supported for
this catalog");
+ throw new ForbiddenException(
+ "Access Delegation is not enabled for this catalog. Please consult
applicable "
+ + "documentation for the catalog config property '%s' to enable
this feature",
+
PolarisConfiguration.ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING.catalogConfig());
Review Comment:
My thinking on the wording was to avoid establishing a precedent of users
starting to blindly set configs recommended by error messages. Admittedly it
adds a bit of friction since this now makes it sound like there might be a more
complicated series of steps to enable the feature rather than simply setting
the config=true, but longer-term we should indeed probably standardize a way to
point at relevant docs from error messages while also maybe making it easy for
users to assess whether they care enough about the pros/cons of different
config choices.
In any case, I totally agree we could have more clarity in the various
config-related error messages, but I'm thinking we can do a bigger refactor to,
for example, try to centralize error-message generation into
`PolarisConfiuration.java` itself. But it'll take some refactoring of callsites.
--
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]
