mchades commented on code in PR #12553:
URL: https://github.com/apache/gravitino/pull/12553#discussion_r3844676825
##########
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:
Fixed in `65da21fe61`. At the JDBC test boundary, a generic converter
fallback backed by `SQLException` is normalized to `ConnectionFailedException`,
while existing domain exceptions are preserved. MySQL-compatible catalogs may
connect during driver metadata validation before reaching `testConnection`, so
that connection-only path is normalized as well. I added common unit coverage
and an existing-catalog MySQL unreachable-endpoint Docker test.
##########
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:
Fixed in `65da21fe61`. `InterruptedException` is now caught separately, the
thread interrupt flag is restored, and the error is wrapped as
`ConnectionFailedException`. The unit test verifies that the flag remains set
and clears it in `finally` to avoid affecting other tests.
##########
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:
Added in `65da21fe61`. The endpoint tests now cover
`IllegalArgumentException`, `NoSuchCatalogException`, and
`CatalogNotInUseException`, verifying HTTP 200, application code, exception
type, message, and the no-stack guarantee.
--
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]
