Copilot commented on code in PR #13013:
URL: https://github.com/apache/gravitino/pull/13013#discussion_r3958747593
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueClientProvider.java:
##########
@@ -92,6 +96,31 @@ public static GlueClient buildClient(Map<String, String>
config) {
return builder.build();
}
+ /**
+ * Eagerly resolves {@code credentialsProvider} to confirm a usable
credential source exists,
+ * instead of leaving resolution to the first real Glue API call. Without
this check, a catalog
+ * created with no static credentials and no usable default-chain source
(env vars, instance
+ * profile, etc.) is stored successfully and then fails on every operation
with a raw AWS SDK
+ * error that never mentions this connector's own credential properties.
+ *
+ * @throws IllegalArgumentException if no credentials can be resolved
+ */
+ @VisibleForTesting
+ static void validateCredentials(AwsCredentialsProvider credentialsProvider) {
+ try {
+ credentialsProvider.resolveCredentials();
+ } catch (SdkClientException e) {
+ throw new IllegalArgumentException(
+ String.format(
+ "No usable AWS credentials found for the Glue catalog. Set both
'%s' and '%s' "
+ + "catalog properties for static authentication, or ensure
the default AWS "
+ + "credential chain (environment variables, instance
profile, web identity "
+ + "token, etc.) can resolve credentials.",
+ GlueConstants.AWS_ACCESS_KEY_ID,
GlueConstants.AWS_SECRET_ACCESS_KEY),
Review Comment:
`validateCredentials` converts *any* `SdkClientException` into “No usable
AWS credentials…”, but `SdkClientException` during `resolveCredentials()` can
also represent non-credential problems (e.g., IMDS/network/endpoint issues
while probing providers). This can produce a misleading top-level message even
though the cause is attached. Consider aligning this with the runtime path:
only emit the “no credentials” message for recognized credential-chain
exhaustion (e.g., via `GlueExceptionConverter.isCredentialFailure(e)`), and
otherwise rethrow/wrap with a message that reflects resolution failure without
asserting “no usable credentials.”
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueExceptionConverter.java:
##########
@@ -30,8 +31,41 @@
/** Converts AWS Glue SDK exceptions to Gravitino exceptions. */
final class GlueExceptionConverter {
+ private static final String NO_CREDENTIALS_MARKER = "Unable to load
credentials";
Review Comment:
Credential-failure detection is based on a substring match against the
exception message, which is brittle across AWS SDK versions/providers and can
lead to missed translations (different wording) or accidental matches
(unrelated “Unable to load credentials …” text). A more robust approach is to
check multiple known markers (e.g., the common “Unable to load credentials from
any of the providers”) and/or inspect nested causes/messages (some providers
wrap). If message matching is the only option, tightening the predicate (e.g.,
a more specific marker) and checking `e.getCause()` can reduce false
positives/negatives.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueExceptionConverter.java:
##########
@@ -30,8 +31,41 @@
/** Converts AWS Glue SDK exceptions to Gravitino exceptions. */
final class GlueExceptionConverter {
+ private static final String NO_CREDENTIALS_MARKER = "Unable to load
credentials";
+
private GlueExceptionConverter() {}
+ /**
+ * Whether {@code e} is the AWS SDK's default-credential-chain-exhausted
error, which otherwise
+ * surfaces as a raw {@link SdkClientException} listing SDK-internal
credential sources instead of
+ * this connector's own {@code aws-access-key-id} / {@code
aws-secret-access-key} properties.
+ *
+ * @param e the client exception raised while calling AWS Glue
+ * @return true if {@code e} is a credential-resolution failure
+ */
+ static boolean isCredentialFailure(SdkClientException e) {
+ return e.getMessage() != null &&
e.getMessage().contains(NO_CREDENTIALS_MARKER);
+ }
+
+ /**
+ * Converts a credential-resolution {@link SdkClientException} into a
message that names this
+ * connector's own credential properties, so operators are not left guessing
which environment
+ * variable or IAM role the raw SDK message intended.
+ *
+ * @param e the credential-resolution failure
+ * @param context description of the operation context for error messages
+ * @return a Gravitino runtime exception with an actionable message
+ */
+ static RuntimeException toCredentialException(SdkClientException e, String
context) {
+ return new RuntimeException(
+ String.format(
+ "Failed to authenticate with AWS Glue while %s. No usable AWS
credentials were "
+ + "found. Set both '%s' and '%s' catalog properties, or ensure
the default AWS "
+ + "credential chain can resolve credentials.",
+ context, GlueConstants.AWS_ACCESS_KEY_ID,
GlueConstants.AWS_SECRET_ACCESS_KEY),
+ e);
+ }
Review Comment:
The new message-matching and formatting logic in `isCredentialFailure` /
`toCredentialException` is central to the PR’s behavior but isn’t directly
unit-tested here (current tests cover it indirectly via `listSchemas`). Adding
focused unit tests for (1) message matching (positive/negative, null message,
nested-cause message) and (2) the formatted output containing the provided
`context` and both property names would make regressions in this translation
logic much easier to catch.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueExceptionConverter.java:
##########
@@ -30,8 +31,41 @@
/** Converts AWS Glue SDK exceptions to Gravitino exceptions. */
final class GlueExceptionConverter {
+ private static final String NO_CREDENTIALS_MARKER = "Unable to load
credentials";
+
private GlueExceptionConverter() {}
+ /**
+ * Whether {@code e} is the AWS SDK's default-credential-chain-exhausted
error, which otherwise
+ * surfaces as a raw {@link SdkClientException} listing SDK-internal
credential sources instead of
+ * this connector's own {@code aws-access-key-id} / {@code
aws-secret-access-key} properties.
+ *
+ * @param e the client exception raised while calling AWS Glue
+ * @return true if {@code e} is a credential-resolution failure
+ */
+ static boolean isCredentialFailure(SdkClientException e) {
+ return e.getMessage() != null &&
e.getMessage().contains(NO_CREDENTIALS_MARKER);
+ }
Review Comment:
The new message-matching and formatting logic in `isCredentialFailure` /
`toCredentialException` is central to the PR’s behavior but isn’t directly
unit-tested here (current tests cover it indirectly via `listSchemas`). Adding
focused unit tests for (1) message matching (positive/negative, null message,
nested-cause message) and (2) the formatted output containing the provided
`context` and both property names would make regressions in this translation
logic much easier to catch.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java:
##########
@@ -215,7 +218,7 @@ public GlueSchema createSchema(
applyCatalogId(catalogId, req::catalogId);
try {
- glueClient.createDatabase(req.build());
+ callGlue(() -> glueClient.createDatabase(req.build()), "schema " +
ident.name());
} catch (GlueException e) {
throw GlueExceptionConverter.toSchemaException(e, "schema " +
ident.name());
Review Comment:
The `context` string is interpolated into messages as “while %s” (see
`toCredentialException`), so values like `"schema " + ident.name()` yield
awkward phrasing (“while schema X”). Using a verb phrase (e.g., “creating
schema X”, “updating schema X”, “loading table X”) will make the actionable
error read cleanly and consistently across call sites.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueExceptionConverter.java:
##########
@@ -30,8 +31,41 @@
/** Converts AWS Glue SDK exceptions to Gravitino exceptions. */
final class GlueExceptionConverter {
+ private static final String NO_CREDENTIALS_MARKER = "Unable to load
credentials";
+
private GlueExceptionConverter() {}
+ /**
+ * Whether {@code e} is the AWS SDK's default-credential-chain-exhausted
error, which otherwise
+ * surfaces as a raw {@link SdkClientException} listing SDK-internal
credential sources instead of
+ * this connector's own {@code aws-access-key-id} / {@code
aws-secret-access-key} properties.
+ *
+ * @param e the client exception raised while calling AWS Glue
+ * @return true if {@code e} is a credential-resolution failure
+ */
+ static boolean isCredentialFailure(SdkClientException e) {
+ return e.getMessage() != null &&
e.getMessage().contains(NO_CREDENTIALS_MARKER);
+ }
Review Comment:
Credential-failure detection is based on a substring match against the
exception message, which is brittle across AWS SDK versions/providers and can
lead to missed translations (different wording) or accidental matches
(unrelated “Unable to load credentials …” text). A more robust approach is to
check multiple known markers (e.g., the common “Unable to load credentials from
any of the providers”) and/or inspect nested causes/messages (some providers
wrap). If message matching is the only option, tightening the predicate (e.g.,
a more specific marker) and checking `e.getCause()` can reduce false
positives/negatives.
--
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]
