PatrickRen commented on code in PR #22924:
URL: https://github.com/apache/flink/pull/22924#discussion_r1262307415
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/api/EnvironmentTest.java:
##########
@@ -25,14 +25,22 @@
import org.apache.flink.streaming.api.environment.StreamExecutionEnvironment;
import org.apache.flink.table.api.bridge.java.StreamTableEnvironment;
import org.apache.flink.table.api.config.TableConfigOptions;
+import org.apache.flink.table.api.internal.TableEnvironmentImpl;
+import org.apache.flink.table.catalog.listener.CatalogModificationEvent;
+import org.apache.flink.table.catalog.listener.CatalogModificationListener;
+import
org.apache.flink.table.catalog.listener.CatalogModificationListenerFactory;
import org.apache.flink.types.Row;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
Review Comment:
Nit: we can remove the `public` keyword before all test cases if we update
the framework to JUnit 5
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/api/EnvironmentTest.java:
##########
@@ -25,14 +25,22 @@
import org.apache.flink.streaming.api.environment.StreamExecutionEnvironment;
import org.apache.flink.table.api.bridge.java.StreamTableEnvironment;
import org.apache.flink.table.api.config.TableConfigOptions;
+import org.apache.flink.table.api.internal.TableEnvironmentImpl;
+import org.apache.flink.table.catalog.listener.CatalogModificationEvent;
+import org.apache.flink.table.catalog.listener.CatalogModificationListener;
+import
org.apache.flink.table.catalog.listener.CatalogModificationListenerFactory;
import org.apache.flink.types.Row;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
import java.time.Duration;
+import java.util.Arrays;
import java.util.concurrent.ExecutionException;
+import java.util.stream.Collectors;
import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
Review Comment:
Please rewrite assertions using AssertJ (`assertThat`) to align with other
test cases.
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/factories/TableFactoryUtil.java:
##########
@@ -164,4 +170,30 @@ public static boolean isLegacyConnectorOptions(
}
}
}
+
+ /** Find and create modification listener list from configuration. */
+ public static List<CatalogModificationListener>
findCatalogModificationListenerList(
+ final ReadableConfig configuration, final ClassLoader classLoader)
{
Review Comment:
I found 4 usages of this method, but the test case only covers one of them
(the `TableEnvironmentImpl` one). What about adding cases for other usages? I'm
a bit concern if the `configuration` is passed in correctly end-to-end (like
user sets them in SQL client session, TableEnv etc.), instead of being
intercepted by a hidden `new Configuration()` that messes everything up.
--
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]
