yuqi1129 commented on code in PR #9735:
URL: https://github.com/apache/gravitino/pull/9735#discussion_r2727452500
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnectorFactory.java:
##########
@@ -117,13 +137,70 @@ public Connector create(
}
GravitinoStoredProcedureFactory gravitinoStoredProcedureFactory =
new GravitinoStoredProcedureFactory(catalogConnectorManager,
metalake);
- return new GravitinoSystemConnector(gravitinoStoredProcedureFactory);
+ return createSystemConnector(gravitinoStoredProcedureFactory);
}
}
- @VisibleForTesting
- Supplier<GravitinoAdminClient> clientProvider() {
- return () -> null;
+ protected GravitinoConnector createConnector(CatalogConnectorContext
connectorContext) {
+ return new GravitinoConnector(connectorContext);
+ }
+
+ protected GravitinoSystemConnector createSystemConnector(
+ GravitinoStoredProcedureFactory storedProcedureFactory) {
+ return new GravitinoSystemConnector(storedProcedureFactory);
+ }
+
+ protected String getTrinoCatalogName(String metalakeName, String
catalogName) {
+ return "\"" + metalakeName + "." + catalogName + "\"";
+ }
+
+ private void checkTrinoSpiVersion(ConnectorContext context, GravitinoConfig
config) {
+ String spiVersion = context.getSpiVersion();
+
+ trinoVersion = Integer.parseInt(spiVersion);
+ if (trinoVersion < getMinSupportTrinoSpiVersion()
+ || trinoVersion > getMaxSupportTrinoSpiVersion()) {
+ Boolean skipTrinoVersionValidation =
config.isSkipTrinoVersionValidation();
Review Comment:
Doesn't `SkipTrinoVersionValidatio` mean we will not execute
`checkTrinoSpiVersion` regardless of the Trino version?
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnector.java:
##########
@@ -47,19 +47,20 @@
public class GravitinoConnector implements Connector {
private final NameIdentifier catalogIdentifier;
- private final CatalogConnectorContext catalogConnectorContext;
+ protected final CatalogConnectorContext catalogConnectorContext;
Review Comment:
Why do we use `protected` access level here? I do not see any access from
subclasses or classes within the packages.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/util/json/BlockJsonSerde.java:
##########
@@ -67,9 +67,7 @@ public void serialize(
throws IOException {
// Encoding name is length prefixed as are many block encodings
SliceOutput output =
- new DynamicSliceOutput(
- toIntExact(
- block.getSizeInBytes() + block.getEncodingName().length() +
(2 * Integer.BYTES)));
+ new DynamicSliceOutput(toIntExact(block.getSizeInBytes() + (2 *
Integer.BYTES) + 1024));
Review Comment:
You may need to add some comments about why the value of
`block.getEncodingName().length()` is a fixed one
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnectorFactory.java:
##########
@@ -45,6 +45,8 @@
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 int MAX_SUPPORT_TRINO_SPI_VERSION = 440;
Review Comment:
I remembered that the maximum version is not 440, am I right?
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnectorFactory.java:
##########
@@ -117,13 +137,70 @@ public Connector create(
}
GravitinoStoredProcedureFactory gravitinoStoredProcedureFactory =
new GravitinoStoredProcedureFactory(catalogConnectorManager,
metalake);
- return new GravitinoSystemConnector(gravitinoStoredProcedureFactory);
+ return createSystemConnector(gravitinoStoredProcedureFactory);
}
}
- @VisibleForTesting
- Supplier<GravitinoAdminClient> clientProvider() {
- return () -> null;
+ protected GravitinoConnector createConnector(CatalogConnectorContext
connectorContext) {
+ return new GravitinoConnector(connectorContext);
+ }
+
+ protected GravitinoSystemConnector createSystemConnector(
+ GravitinoStoredProcedureFactory storedProcedureFactory) {
+ return new GravitinoSystemConnector(storedProcedureFactory);
+ }
+
+ protected String getTrinoCatalogName(String metalakeName, String
catalogName) {
+ return "\"" + metalakeName + "." + catalogName + "\"";
+ }
+
+ private void checkTrinoSpiVersion(ConnectorContext context, GravitinoConfig
config) {
+ String spiVersion = context.getSpiVersion();
+
+ trinoVersion = Integer.parseInt(spiVersion);
+ if (trinoVersion < getMinSupportTrinoSpiVersion()
+ || trinoVersion > getMaxSupportTrinoSpiVersion()) {
+ Boolean skipTrinoVersionValidation =
config.isSkipTrinoVersionValidation();
+ if (!skipTrinoVersionValidation) {
+ String errmsg =
+ String.format(
+ "Unsupported Trino-%s version. The Supported version for the
Gravitino-Trino-connector from Trino-%d to Trino-%d."
+ + "Maybe you can set
gravitino.trino.skip-version-validation to skip version validation.",
+ trinoVersion, getMinSupportTrinoSpiVersion(),
getMaxSupportTrinoSpiVersion());
+ throw new
TrinoException(GravitinoErrorCode.GRAVITINO_UNSUPPORTED_TRINO_VERSION, errmsg);
+ } else {
+ LOG.warn(
+ "The version {} has not undergone thorough testing with Gravitino,
there may be compatiablity problem.",
+ trinoVersion);
+ }
+ }
+
+ if (!config.singleMetalakeMode()) {
Review Comment:
Merge these two `if`
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorManager.java:
##########
@@ -369,24 +377,32 @@ public Set<String> getUsedMetalakes() {
* @param connectorName the name of the connector
* @param config the Gravitino configuration
* @param context the Trino connector context
- * @return the created connector
+ * @return the created catalog connector context
*/
- public Connector createConnector(
+ public CatalogConnectorContext createCatalogConnectorContext(
String connectorName, GravitinoConfig config, ConnectorContext context) {
try {
String catalogConfig = config.getCatalogConfig();
GravitinoCatalog catalog = GravitinoCatalog.fromJson(catalogConfig);
+ if (this.config.singleMetalakeMode()
+ && !Strings.isNullOrEmpty(targetMetalake)
Review Comment:
Please use `Strings.isNotBlank(...)` instead.
##########
trino-connector/trino-connector/build.gradle.kts:
##########
@@ -26,47 +26,32 @@ repositories {
mavenCentral()
}
+var trinoVersion = 435
+val trinoVersionProvider =
+ providers.gradleProperty("trinoVersion").map { it.toInt() }.orElse(435)
+trinoVersion = trinoVersionProvider.get()
Review Comment:
Why not use `val trinoVersion = trinoVersionProvider.get()` directly here?
--
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]
