Copilot commented on code in PR #13057:
URL: https://github.com/apache/gravitino/pull/13057#discussion_r3975424450
##########
catalogs/catalog-lakehouse-iceberg/src/test/java/org/apache/gravitino/catalog/lakehouse/iceberg/integration/test/CatalogIcebergKerberosHiveIT.java:
##########
@@ -333,6 +333,16 @@ void testIcebergWithKerberos() {
adminClient =
GravitinoAdminClient.builder(serverUri).withKerberosAuth(provider).build();
String metalakeName = GravitinoITUtils.genRandomName("test_metalake");
+ String catalogName = GravitinoITUtils.genRandomName("test_catalog");
+ String schemaName = GravitinoITUtils.genRandomName("test_schema");
+ String warehousePath =
+ "/user/hive/" +
GravitinoITUtils.genRandomName("warehouse-catalog-iceberg");
+ String warehouse =
+ String.format(
+ "hdfs://%s:%d%s/",
+ kerberosHiveContainer.getContainerIpAddress(),
+ HiveContainer.HDFS_DEFAULTFS_PORT,
+ warehousePath);
Review Comment:
The test now builds `warehouse` using `HiveContainer.HDFS_DEFAULTFS_PORT`,
but hardcodes `location` to `hdfs://localhost:9000...`. This inconsistency (and
the magic port `9000`) makes the test environment-dependent and harder to
maintain. Prefer deriving `location` from the same host/port inputs (or the
same computed URI components) used for `warehouse`.
##########
catalogs/catalog-lakehouse-iceberg/src/test/java/org/apache/gravitino/catalog/lakehouse/iceberg/integration/test/CatalogIcebergKerberosHiveIT.java:
##########
@@ -351,22 +361,39 @@ void testIcebergWithKerberos() {
properties.put(IcebergConfig.CATALOG_BACKEND.getKey(), TYPE);
properties.put(IcebergConfig.CATALOG_URI.getKey(), URIS);
- properties.put(IcebergConfig.CATALOG_WAREHOUSE.getKey(), WAREHOUSE);
- properties.put("location",
"hdfs://localhost:9000/user/hive/warehouse-catalog-iceberg");
+ properties.put(IcebergConfig.CATALOG_WAREHOUSE.getKey(), warehouse);
+ properties.put("location", "hdfs://localhost:9000" + warehousePath);
Review Comment:
The test now builds `warehouse` using `HiveContainer.HDFS_DEFAULTFS_PORT`,
but hardcodes `location` to `hdfs://localhost:9000...`. This inconsistency (and
the magic port `9000`) makes the test environment-dependent and harder to
maintain. Prefer deriving `location` from the same host/port inputs (or the
same computed URI components) used for `warehouse`.
##########
server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java:
##########
@@ -651,10 +661,25 @@ public void
testExecutorDenialDispatchesEventAndSetsFlag() throws Throwable {
/**
* When {@code checkCurrentUser} throws {@link ForbiddenException} (user is
not a metalake
* member), the interceptor must dispatch an {@link
AuthorizationDenialFailureEvent} and set
- * {@code operationFailureFired}.
+ * {@code operationFailureFired}, while logging the original exception for
server-side diagnosis.
*/
@Test
- public void testForbiddenExceptionDispatchesEventAndSetsFlag() throws
Throwable {
+ public void testForbiddenExceptionDispatchesEventSetsFlagAndLogsThrowable()
throws Throwable {
+ String loggerName =
+ GravitinoInterceptionService.class.getName() +
"$MetadataAuthorizationMethodInterceptor";
+ LoggerContext loggerContext =
+ (LoggerContext)
+
LogManager.getContext(GravitinoInterceptionService.class.getClassLoader(),
false);
+ AbstractConfiguration configuration = (AbstractConfiguration)
loggerContext.getConfiguration();
+ CaptureAppender captureAppender = new
CaptureAppender("authorizationCapture");
+ captureAppender.start();
+ configuration.addAppender(captureAppender);
+ LoggerConfig loggerConfig = new LoggerConfig(loggerName, Level.WARN,
false);
+ loggerConfig.addAppender(captureAppender, Level.WARN, null);
+ configuration.addLogger(loggerName, loggerConfig);
+ loggerContext.updateLoggers();
Review Comment:
This test mutates the global `LoggerContext` configuration. If other tests
run in parallel (or if this setup throws before entering the `try`/`finally`),
it can leak configuration and cause cross-test interference. Consider wrapping
the *entire* logger setup in a `try/finally`, and/or capturing and restoring
any pre-existing `LoggerConfig` for `loggerName` to ensure the original logging
configuration is fully restored.
##########
server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java:
##########
@@ -700,6 +725,13 @@ public void
testForbiddenExceptionDispatchesEventAndSetsFlag() throws Throwable
AuthorizationDenialFailureEvent event = captor.getValue();
assertEquals("outsider", event.user());
Assertions.assertTrue(RequestContext.isOperationFailureFired());
+ assertEquals(1, captureAppender.getEvents().size());
+ assertSame(forbiddenException,
captureAppender.getEvents().get(0).getThrown());
Review Comment:
This assertion is brittle: the interceptor path may emit more than one WARN
log (now or in future edits), which would cause a non-behavioral test failure.
Consider asserting that at least one captured event contains the expected
throwable (and optionally the expected message/logger name), rather than
requiring an exact size of 1.
##########
catalogs/catalog-lakehouse-paimon/src/test/java/org/apache/gravitino/catalog/lakehouse/paimon/integration/test/CatalogPaimonKerberosFilesystemIT.java:
##########
@@ -284,4 +284,10 @@ private static Column[] createColumns() {
Column col3 = Column.of(FILESYSTEM_COL_NAME3, Types.StringType.get(),
"col_3_comment");
return new Column[] {col1, col2, col3};
}
+
+ private static void assertPublicClientInternalError(
+ RuntimeException exception, String expectedMessageFragment) {
+ Assertions.assertEquals(RuntimeException.class, exception.getClass());
+
Assertions.assertTrue(exception.getMessage().contains(expectedMessageFragment));
+ }
Review Comment:
This helper (and identical copies in multiple IT classes in this PR)
duplicates the same assertion logic across modules. To reduce repetition and
keep the public-error-contract assertion consistent, consider centralizing this
into a shared integration-test utility (or a shared static helper) and reusing
it across the Kerberos/JDBC IT suites.
##########
server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java:
##########
@@ -1031,4 +1063,21 @@ public void handleMetadataOwnerChange(
@Override
public void close() throws IOException {}
}
+
+ private static class CaptureAppender extends AbstractAppender {
+ private final List<LogEvent> events = new ArrayList<>();
+
+ CaptureAppender(String name) {
+ super(name, null, PatternLayout.createDefaultLayout(), true, null);
+ }
+
+ @Override
+ public void append(LogEvent event) {
+ events.add(event.toImmutable());
+ }
Review Comment:
Log4j appenders can be invoked concurrently, and `ArrayList` is not
thread-safe for concurrent writes. To prevent flaky failures in parallel test
execution or async logging scenarios, store events in a thread-safe collection
(e.g., `Collections.synchronizedList(...)` or a concurrent queue).
--
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]
