hqbhoho commented on code in PR #9735:
URL: https://github.com/apache/gravitino/pull/9735#discussion_r2734745927
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorManager.java:
##########
@@ -127,19 +130,16 @@ public void config(GravitinoConfig config,
GravitinoAdminClient client) {
/**
* Starts the catalog connector manager with the specified Trino connector
context.
*
- * @param context the Trino connector context
* @throws Exception if the catalog connector manager fails to start
*/
- public void start(ConnectorContext context) throws Exception {
- catalogRegister.init(context, config);
- if (catalogRegister.isCoordinator()) {
- executorService.scheduleWithFixedDelay(
- this::loadMetalake,
- metadataUpdateIntervalSecond,
- metadataUpdateIntervalSecond,
- TimeUnit.SECONDS);
- }
-
+ public void start() throws Exception {
+ catalogRegister.init(config);
+ executorService.scheduleWithFixedDelay(
+ this::loadMetalake,
+ metadataUpdateIntervalSecond,
+ metadataUpdateIntervalSecond,
+ TimeUnit.SECONDS);
+ gravitinoClient.close();
Review Comment:
The `gravitinoClient` should not close here. Maybe in `shutdown`
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/util/json/BlockJsonSerde.java:
##########
@@ -37,6 +37,7 @@
*/
public final class BlockJsonSerde {
private static final String BLOCK_SERDE_UTIL_CLASS_NAME =
"io.trino.block.BlockSerdeUtil";
+ private static final int DEFAULT_BLOCK_ENCODING_NAME_LENGTH = 1024;
Review Comment:
We set the value to 16 or 32 to accommodate the maximum length of 14
(VariableWidthBlockEncoding)
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/system/GravitinoSystemConnector.java:
##########
@@ -129,13 +147,18 @@ public ConnectorSplitSource getSplits(
SchemaTableName tableName =
((GravitinoSystemConnectorMetadata.SystemTableHandle)
connectorTableHandle).getName();
- return new FixedSplitSource(new Split(tableName));
+ return new FixedSplitSource(createSplit(tableName));
+ }
+
+ protected ConnectorSplit createSplit(SchemaTableName tableName) {
+ throw new RuntimeException("Should be overridden in subclass");
Review Comment:
Maybe throw a `NOT_SUPPORT` exception or give a default implementation
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnectorFactory.java:
##########
@@ -117,13 +137,75 @@ 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) {
+ throw new RuntimeException("Should be overridden in subclass");
+ }
+
+ 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);
+
+ // check catalog name with metalake are supported in this trino version
+ if (!config.singleMetalakeMode() && !supportCatalogNameWithMetalake()) {
+ String errmsg =
+ String.format(
+ "The trino-connector-%s-%s does not support catalog name with
metalake.",
+ getMinSupportTrinoSpiVersion(), getMaxSupportTrinoSpiVersion());
+ throw new
TrinoException(GravitinoErrorCode.GRAVITINO_UNSUPPORTED_TRINO_VERSION, errmsg);
+ }
+
+ // skip version validation
+ boolean spiVersionCheck = config.isSkipTrinoVersionValidation();
+ if (spiVersionCheck) {
+ if (trinoVersion < getMinSupportTrinoSpiVersion()
+ || trinoVersion > getMaxSupportTrinoSpiVersion()) {
+ LOG.warn(
+ "The version {} has not undergone thorough testing with Gravitino,
there may be compatibility problem.",
+ trinoVersion);
+ }
+ return;
+ }
+
+ // version validation
+ if (trinoVersion < getMinSupportTrinoSpiVersion()
+ || trinoVersion > getMaxSupportTrinoSpiVersion()) {
+ 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);
+ }
+ }
+
+ protected boolean supportCatalogNameWithMetalake() {
+ return true;
+ }
+
+ protected int getMinSupportTrinoSpiVersion() {
+ return MIN_SUPPORT_TRINO_SPI_VERSION;
+ }
+
+ protected int getMaxSupportTrinoSpiVersion() {
+ return MAX_SUPPORT_TRINO_SPI_VERSION;
+ }
+
+ @SuppressWarnings("deprecation")
+ protected boolean isCoordinator(ConnectorContext connectorContext) {
+ return connectorContext.getNodeManager().getCurrentNode().isCoordinator();
Review Comment:
We should be consistent here. Shall we use a default implementation or throw
a `NOT_SUPPORT` exception.
##########
trino-connector/trino-connector/src/test/java/org/apache/gravitino/trino/connector/TestGravitinoConnector.java:
##########
@@ -189,38 +151,51 @@ public void testAlterTable() throws Exception {
createTestTable(fullTableName1);
- // test add column and drop column, but the memory connector is not
supported these operations.
- assertQueryFails(
- String.format("alter table %s add column if not exists c varchar",
fullTableName1),
- format("This connector does not support adding columns"));
-
- assertQueryFails(
- String.format("alter table %s drop column a", fullTableName1),
- format("This connector does not support dropping columns"));
-
// test set table comment
assertUpdate(String.format("comment on table %s is 'test table comments'",
fullTableName1));
assertThat((String) computeScalar("show create table " + fullTableName1))
.contains("COMMENT 'test table comments'");
- // test rename column, but the memory connector is not supported these
operations.
- assertQueryFails(
- String.format("alter table %s rename column a to c ", fullTableName1),
- format("This connector does not support renaming columns"));
-
- assertQueryFails(
- String.format("alter table %s alter column a set DATA TYPE int",
fullTableName1),
- format("This connector does not support setting column types"));
-
// test set column comment
assertUpdate(String.format("comment on column %s.a is 'test column
comments'", fullTableName1));
assertThat((String) computeScalar("show create table " + fullTableName1))
.contains("COMMENT 'test column comments'");
+ // test add column and drop column, but the memory connector is not
supported these operations.
+ if (trinoVersion < 469) {
Review Comment:
Maybe we can use a `SUPPORT_ADD_COLUMN` test label to control the test
behavior
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnectorFactory.java:
##########
@@ -117,13 +137,75 @@ 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) {
+ throw new RuntimeException("Should be overridden in subclass");
Review Comment:
Maybe throw a `NOT_SUPPORT` exception or give a default implementation
--
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]
