yuqi1129 commented on code in PR #12553:
URL: https://github.com/apache/gravitino/pull/12553#discussion_r3843487054
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java:
##########
@@ -155,13 +154,7 @@ public void initialize(
}
@Override
- public void testConnection(
- NameIdentifier catalogIdent,
- Catalog.Type type,
- String provider,
- String comment,
- Map<String, String> properties)
- throws Exception {
+ public void testConnection(NameIdentifier catalogIdent) {
Review Comment:
`getDatabases` can fail with `SdkClientException` for DNS, connect timeout,
TLS, or client-side credential-chain failures; `SdkClientException` is not a
`GlueException`. Those are central connection-test failures, but they currently
escape as a generic runtime exception and the REST layer returns
`INTERNAL_ERROR` instead of `CONNECTION_FAILED`. Please map the SDK client-side
failures as well and add a focused negative-path test.
##########
server/src/test/java/org/apache/gravitino/server/web/rest/TestCatalogOperations.java:
##########
@@ -339,6 +340,77 @@ public void testConnection() {
ErrorResponse errorResponse = resp1.readEntity(ErrorResponse.class);
Assertions.assertEquals(ErrorConstants.INTERNAL_ERROR_CODE,
errorResponse.getCode());
Assertions.assertEquals(RuntimeException.class.getSimpleName(),
errorResponse.getType());
+
+ ConnectionFailedException legacyFailure =
+ new ConnectionFailedException(
+ new IllegalStateException("database connection detail"),
"connection failed");
+ doThrow(legacyFailure).when(manager).testConnection(any(), any(), any(),
any(), any());
+ Response failedResponse =
+ target("/metalakes/metalake1/catalogs/testConnection")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .post(Entity.entity(req, MediaType.APPLICATION_JSON_TYPE));
+
+ ErrorResponse connectionError =
failedResponse.readEntity(ErrorResponse.class);
+ Assertions.assertEquals(ErrorConstants.CONNECTION_FAILED_CODE,
connectionError.getCode());
+ Assertions.assertNotNull(connectionError.getStack());
+ Assertions.assertTrue(
+ String.join("\n", connectionError.getStack()).contains("database
connection detail"));
+ }
+
+ @Test
+ public void testExistingCatalogConnection() {
+ doNothing().when(manager).testConnection(any(NameIdentifier.class));
+ Response response =
+ target("/metalakes/metalake1/catalogs/catalog1/testConnection")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .post(null);
+
+ Assertions.assertEquals(Response.Status.OK.getStatusCode(),
response.getStatus());
+ Assertions.assertEquals(0,
response.readEntity(BaseResponse.class).getCode());
+
+ doThrow(new ConnectionFailedException("sanitized failure"))
+ .when(manager)
+ .testConnection(any(NameIdentifier.class));
+ Response failedResponse =
+ target("/metalakes/metalake1/catalogs/catalog1/testConnection")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .post(null);
+
+ Assertions.assertEquals(Response.Status.OK.getStatusCode(),
failedResponse.getStatus());
+ ErrorResponse errorResponse =
failedResponse.readEntity(ErrorResponse.class);
+ Assertions.assertEquals(ErrorConstants.CONNECTION_FAILED_CODE,
errorResponse.getCode());
+ Assertions.assertEquals("sanitized failure", errorResponse.getMessage());
+ Assertions.assertNull(errorResponse.getStack());
+
+ doThrow(new UnsupportedOperationException("unsupported"))
+ .when(manager)
+ .testConnection(any(NameIdentifier.class));
+ Response unsupportedResponse =
+ target("/metalakes/metalake1/catalogs/catalog1/testConnection")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .post(null);
+ ErrorResponse unsupported =
unsupportedResponse.readEntity(ErrorResponse.class);
+ Assertions.assertEquals(ErrorConstants.UNSUPPORTED_OPERATION_CODE,
unsupported.getCode());
+ Assertions.assertNull(unsupported.getStack());
+
+ doThrow(new RuntimeException("unexpected failure"))
+ .when(manager)
+ .testConnection(any(NameIdentifier.class));
+ Response internalErrorResponse =
+ target("/metalakes/metalake1/catalogs/catalog1/testConnection")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .post(null);
+ Assertions.assertEquals(
+ INTERNAL_SERVER_ERROR.getStatusCode(),
internalErrorResponse.getStatus());
+ ErrorResponse internalError =
internalErrorResponse.readEntity(ErrorResponse.class);
+ Assertions.assertEquals(ErrorConstants.INTERNAL_ERROR_CODE,
internalError.getCode());
+ Assertions.assertEquals("unexpected failure", internalError.getMessage());
+ Assertions.assertNull(internalError.getStack());
Review Comment:
The existing-catalog exception handler also has externally visible branches
for `IllegalArgumentException`, `NoSuchCatalogException`, and
`CatalogNotInUseException`, but this test currently covers only success,
connection failure, unsupported operation, and an unexpected runtime exception.
Please add endpoint assertions for those remaining documented branches,
including HTTP/application code and the no-stack guarantee.
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogOperations.java:
##########
@@ -235,18 +234,9 @@ public NameIdentifier[] listSchemas(Namespace namespace)
throws NoSuchCatalogExc
* Performs `show databases` operation to check if the JDBC connection is
valid.
*
* @param catalogIdent the name of the catalog.
- * @param type the type of the catalog.
- * @param provider the provider of the catalog.
- * @param comment the comment of the catalog.
- * @param properties the properties of the catalog.
*/
@Override
- public void testConnection(
- NameIdentifier catalogIdent,
- Catalog.Type type,
- String provider,
- String comment,
- Map<String, String> properties) {
+ public void testConnection(NameIdentifier catalogIdent) {
Review Comment:
The JDBC exception converters only classify a subset of failures as
`ConnectionFailedException`. For example, the default converter maps `Access
denied`, but a MySQL `Communications link failure` or connection refusal
normally remains a generic `GravitinoRuntimeException`; Doris and StarRocks
have the same gap. Consequently this endpoint can return `INTERNAL_ERROR` for
an actual connectivity failure (PostgreSQL happens to work because it maps
SQLSTATE class `08`). Please normalize non-domain failures at this test
boundary and add existing-catalog unreachable-endpoint tests; the current
wrong-password pre-create tests do not cover this case.
##########
catalogs/catalog-kafka/src/main/java/org/apache/gravitino/catalog/kafka/KafkaCatalogOperations.java:
##########
@@ -185,12 +184,7 @@ public NameIdentifier[] listTopics(Namespace namespace)
throws NoSuchSchemaExcep
}
@Override
- public void testConnection(
- NameIdentifier catalogIdent,
- Catalog.Type type,
- String provider,
- String comment,
- Map<String, String> properties) {
+ public void testConnection(NameIdentifier catalogIdent) {
Review Comment:
`Future.get()` may throw `InterruptedException`. Catching it through
`Exception` loses the interruption contract. Please catch
`InterruptedException` separately, restore the flag with
`Thread.currentThread().interrupt()`, then wrap it as the connection failure. A
unit test should assert that the interrupt flag is preserved.
--
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]
