Copilot commented on code in PR #12635:
URL: https://github.com/apache/gravitino/pull/12635#discussion_r3860833834
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnectorFactory.java:
##########
@@ -45,14 +46,12 @@
import org.apache.gravitino.trino.connector.system.GravitinoSystemConnector;
import
org.apache.gravitino.trino.connector.system.storedprocedure.GravitinoStoredProcedureFactory;
import
org.apache.gravitino.trino.connector.system.table.GravitinoSystemTableFactory;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/** Gravitino connector factory. */
public class GravitinoConnectorFactory implements ConnectorFactory {
- private static final Logger LOG =
LoggerFactory.getLogger(GravitinoConnectorFactory.class);
- private static final int MIN_SUPPORT_TRINO_SPI_VERSION = 435;
+ private static final Logger LOG =
Logger.get(GravitinoConnectorFactory.class);
+ private static final int MIN_SUPPORT_TRINO_SPI_VERSION = 440;
private static final int MAX_SUPPORT_TRINO_SPI_VERSION = Integer.MAX_VALUE;
Review Comment:
MIN_SUPPORT_TRINO_SPI_VERSION was bumped to 440, but the repo still has a
dedicated `:trino-connector:trino-connector-435-439` module that compiles the
same sources for Trino 435–439. With this change, that variant will throw
`GRAVITINO_UNSUPPORTED_TRINO_VERSION` at runtime on Trino 435–439 (unless users
set the skip flag), effectively breaking the supported range.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogPropertyConverter.java:
##########
@@ -20,14 +20,13 @@
package org.apache.gravitino.trino.connector.catalog;
import com.google.common.collect.ImmutableMap;
+import io.airlift.log.Logger;
import java.util.HashMap;
import java.util.Map;
import org.apache.gravitino.catalog.property.PropertyConverter;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
public class CatalogPropertyConverter extends PropertyConverter {
- private static final Logger LOG =
LoggerFactory.getLogger(PropertyConverter.class);
+ private static final Logger LOG = Logger.get(PropertyConverter.class);
Review Comment:
The logger is created for `PropertyConverter.class`, which makes log records
appear under the base class rather than `CatalogPropertyConverter`. This makes
log filtering and debugging harder and is inconsistent with the rest of the
connector classes (which use their own class for the logger name).
--
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]
