Split metastore timeout config for connect and read

trinodb · Oct 23, 2023 · 5c5c6e6 · 5c5c6e6
1 parent 90f3ba1
commit 5c5c6e6
Show file tree

Hide file tree

Showing 12 changed files with 78 additions and 28 deletions.
diff --git a/docs/src/main/sphinx/connector/metastores.md b/docs/src/main/sphinx/connector/metastores.md
@@ -74,9 +74,6 @@ are also available. They are discussed later in this topic.
     * - ``hive.metastore-refresh-max-threads``
       - Maximum threads used to refresh cached metastore data.
       - ``10``
-    * - ``hive.metastore-timeout``
-      - Timeout for Hive metastore requests.
-      - ``10s``
     * - ``hive.hide-delta-lake-tables``
       - Controls whether to hide Delta Lake tables in table listings. Currently
         applies only when using the AWS Glue metastore.
@@ -113,6 +110,12 @@ properties:
      - Hive metastore authentication type. Possible values are ``NONE`` or
        ``KERBEROS``.
      - ``NONE``
+   * - ``hive.metastore.thrift.client.connect-timeout``
+     - Socket connect timeout for metastore client.
+     - ``10s``
+   * - ``hive.metastore.thrift.client.read-timeout``
+     - Socket read timeout for metastore client.
+     - ``10s``
    * - ``hive.metastore.thrift.impersonation.enabled``
      - Enable Hive metastore end user impersonation.
      -

diff --git a/...o-delta-lake/src/test/java/io/trino/plugin/deltalake/BaseDeltaLakeConnectorSmokeTest.java b/...o-delta-lake/src/test/java/io/trino/plugin/deltalake/BaseDeltaLakeConnectorSmokeTest.java
@@ -223,7 +223,7 @@ private DistributedQueryRunner createDeltaLakeQueryRunner()
                         .put("delta.metadata.live-files.cache-ttl", TEST_METADATA_CACHE_TTL_SECONDS + "s")
                         .put("hive.metastore-cache-ttl", TEST_METADATA_CACHE_TTL_SECONDS + "s")
                         .put("delta.register-table-procedure.enabled", "true")
-                        .put("hive.metastore-timeout", "1m") // read timed out sometimes happens with the default timeout
+                        .put("hive.metastore.thrift.client.read-timeout", "1m") // read timed out sometimes happens with the default timeout
                         .putAll(deltaStorageConfiguration())
                         .buildOrThrow(),
                 hiveHadoop,

diff --git a/plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/DeltaLakeQueryRunner.java b/plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/DeltaLakeQueryRunner.java
@@ -170,7 +170,7 @@ public static DistributedQueryRunner createS3DeltaLakeQueryRunner(
                         .put("s3.endpoint", minioAddress)
                         .put("s3.path-style-access", "true")
                         .put("s3.streaming.part-size", "5MB") // minimize memory usage
-                        .put("hive.metastore-timeout", "1m") // read timed out sometimes happens with the default timeout
+                        .put("hive.metastore.thrift.client.read-timeout", "1m") // read timed out sometimes happens with the default timeout
                         .putAll(connectorProperties)
                         .buildOrThrow(),
                 testingHadoop,

diff --git a/.../main/java/io/trino/plugin/hive/metastore/thrift/DefaultThriftMetastoreClientFactory.java b/.../main/java/io/trino/plugin/hive/metastore/thrift/DefaultThriftMetastoreClientFactory.java
@@ -54,7 +54,8 @@ public class DefaultThriftMetastoreClientFactory
 {
     private final Optional<SSLContext> sslContext;
     private final Optional<HostAndPort> socksProxy;
-    private final int timeoutMillis;
+    private final int connectTimeoutMillis;
+    private final int readTimeoutMillis;
     private final HiveMetastoreAuthentication metastoreAuthentication;
     private final String hostname;
 
@@ -70,13 +71,15 @@ public class DefaultThriftMetastoreClientFactory
     public DefaultThriftMetastoreClientFactory(
             Optional<SSLContext> sslContext,
             Optional<HostAndPort> socksProxy,
-            Duration timeout,
+            Duration connectTimeout,
+            Duration readTimeout,
             HiveMetastoreAuthentication metastoreAuthentication,
             String hostname)
     {
         this.sslContext = requireNonNull(sslContext, "sslContext is null");
         this.socksProxy = requireNonNull(socksProxy, "socksProxy is null");
-        this.timeoutMillis = toIntExact(timeout.toMillis());
+        this.connectTimeoutMillis = toIntExact(connectTimeout.toMillis());
+        this.readTimeoutMillis = toIntExact(readTimeout.toMillis());
         this.metastoreAuthentication = requireNonNull(metastoreAuthentication, "metastoreAuthentication is null");
         this.hostname = requireNonNull(hostname, "hostname is null");
     }
@@ -95,7 +98,8 @@ public DefaultThriftMetastoreClientFactory(
                         config.getTruststorePath(),
                         Optional.ofNullable(config.getTruststorePassword())),
                 Optional.ofNullable(config.getSocksProxy()),
-                config.getMetastoreTimeout(),
+                config.getConnectTimeout(),
+                config.getReadTimeout(),
                 metastoreAuthentication,
                 nodeManager.getCurrentNode().getHost());
     }
@@ -126,7 +130,7 @@ protected ThriftMetastoreClient create(TransportSupplier transportSupplier, Stri
     private TTransport createTransport(HostAndPort address, Optional<String> delegationToken)
             throws TTransportException
     {
-        return Transport.create(address, sslContext, socksProxy, timeoutMillis, metastoreAuthentication, delegationToken);
+        return Transport.create(address, sslContext, socksProxy, connectTimeoutMillis, readTimeoutMillis, metastoreAuthentication, delegationToken);
     }
 
     private static Optional<SSLContext> buildSslContext(

diff --git a/...trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreConfig.java b/...trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreConfig.java
@@ -30,7 +30,8 @@
 
 public class ThriftMetastoreConfig
 {
-    private Duration metastoreTimeout = new Duration(10, TimeUnit.SECONDS);
+    private Duration connectTimeout = new Duration(10, TimeUnit.SECONDS);
+    private Duration readTimeout = new Duration(10, TimeUnit.SECONDS);
     private HostAndPort socksProxy;
     private int maxRetries = RetryDriver.DEFAULT_MAX_ATTEMPTS - 1;
     private double backoffScaleFactor = RetryDriver.DEFAULT_SCALE_FACTOR;
@@ -54,15 +55,32 @@ public class ThriftMetastoreConfig
     private boolean batchMetadataFetchEnabled = true;
 
     @NotNull
-    public Duration getMetastoreTimeout()
+    public Duration getConnectTimeout()
     {
-        return metastoreTimeout;
+        return connectTimeout;
     }
 
-    @Config("hive.metastore-timeout")
-    public ThriftMetastoreConfig setMetastoreTimeout(Duration metastoreTimeout)
+    @Config("hive.metastore.thrift.client.connect-timeout")
+    @LegacyConfig("hive.metastore-timeout")
+    @ConfigDescription("Socket connect timeout for metastore client")
+    public ThriftMetastoreConfig setConnectTimeout(Duration connectTimeout)
     {
-        this.metastoreTimeout = metastoreTimeout;
+        this.connectTimeout = connectTimeout;
+        return this;
+    }
+
+    @NotNull
+    public Duration getReadTimeout()
+    {
+        return readTimeout;
+    }
+
+    @Config("hive.metastore.thrift.client.read-timeout")
+    @LegacyConfig("hive.metastore-timeout")
+    @ConfigDescription("Socket read timeout for metastore client")
+    public ThriftMetastoreConfig setReadTimeout(Duration readTimeout)
+    {
+        this.readTimeout = readTimeout;
         return this;
     }
 

diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/Transport.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/Transport.java
@@ -36,14 +36,15 @@ public static TTransport create(
             HostAndPort address,
             Optional<SSLContext> sslContext,
             Optional<HostAndPort> socksProxy,
-            int timeoutMillis,
+            int connectTimeoutMillis,
+            int readTimeoutMillis,
             HiveMetastoreAuthentication authentication,
             Optional<String> delegationToken)
             throws TTransportException
     {
         requireNonNull(address, "address is null");
         try {
-            TTransport rawTransport = createRaw(address, sslContext, socksProxy, timeoutMillis);
+            TTransport rawTransport = createRaw(address, sslContext, socksProxy, connectTimeoutMillis, readTimeoutMillis);
             TTransport authenticatedTransport = authentication.authenticate(rawTransport, address.getHost(), delegationToken);
             if (!authenticatedTransport.isOpen()) {
                 authenticatedTransport.open();
@@ -57,7 +58,12 @@ public static TTransport create(
 
     private Transport() {}
 
-    private static TTransport createRaw(HostAndPort address, Optional<SSLContext> sslContext, Optional<HostAndPort> socksProxy, int timeoutMillis)
+    private static TTransport createRaw(
+            HostAndPort address,
+            Optional<SSLContext> sslContext,
+            Optional<HostAndPort> socksProxy,
+            int connectTimeoutMillis,
+            int readTimeoutMillis)
             throws TTransportException
     {
         Proxy proxy = socksProxy
@@ -66,8 +72,8 @@ private static TTransport createRaw(HostAndPort address, Optional<SSLContext> ss
 
         Socket socket = new Socket(proxy);
         try {
-            socket.connect(new InetSocketAddress(address.getHost(), address.getPort()), timeoutMillis);
-            socket.setSoTimeout(timeoutMillis);
+            socket.connect(new InetSocketAddress(address.getHost(), address.getPort()), connectTimeoutMillis);
+            socket.setSoTimeout(readTimeoutMillis);
 
             if (sslContext.isPresent()) {
                 // SSL will connect to the SOCKS address when present

diff --git a/...o-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestThriftMetastoreConfig.java b/...o-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestThriftMetastoreConfig.java
@@ -23,6 +23,7 @@
 import java.nio.file.Path;
 import java.util.Map;
 
+import static io.airlift.configuration.testing.ConfigAssertions.assertDeprecatedEquivalence;
 import static io.airlift.configuration.testing.ConfigAssertions.assertFullMapping;
 import static io.airlift.configuration.testing.ConfigAssertions.assertRecordedDefaults;
 import static io.airlift.configuration.testing.ConfigAssertions.recordDefaults;
@@ -37,7 +38,8 @@ public class TestThriftMetastoreConfig
     public void testDefaults()
     {
         assertRecordedDefaults(recordDefaults(ThriftMetastoreConfig.class)
-                .setMetastoreTimeout(new Duration(10, SECONDS))
+                .setConnectTimeout(new Duration(10, SECONDS))
+                .setReadTimeout(new Duration(10, SECONDS))
                 .setSocksProxy(null)
                 .setMaxRetries(9)
                 .setBackoffScaleFactor(2.0)
@@ -68,7 +70,8 @@ public void testExplicitPropertyMappings()
         Path truststoreFile = Files.createTempFile(null, null);
 
         Map<String, String> properties = ImmutableMap.<String, String>builder()
-                .put("hive.metastore-timeout", "20s")
+                .put("hive.metastore.thrift.client.connect-timeout", "22s")
+                .put("hive.metastore.thrift.client.read-timeout", "44s")
                 .put("hive.metastore.thrift.client.socks-proxy", "localhost:1234")
                 .put("hive.metastore.thrift.client.max-retries", "15")
                 .put("hive.metastore.thrift.client.backoff-scale-factor", "3.0")
@@ -92,7 +95,8 @@ public void testExplicitPropertyMappings()
                 .buildOrThrow();
 
         ThriftMetastoreConfig expected = new ThriftMetastoreConfig()
-                .setMetastoreTimeout(new Duration(20, SECONDS))
+                .setConnectTimeout(new Duration(22, SECONDS))
+                .setReadTimeout(new Duration(44, SECONDS))
                 .setSocksProxy(HostAndPort.fromParts("localhost", 1234))
                 .setMaxRetries(15)
                 .setBackoffScaleFactor(3.0)
@@ -116,4 +120,18 @@ public void testExplicitPropertyMappings()
 
         assertFullMapping(properties, expected);
     }
+
+    @Test
+    public void testLegacyPropertyMappings()
+    {
+        assertDeprecatedEquivalence(
+                ThriftMetastoreConfig.class,
+                Map.of(
+                        "hive.metastore.thrift.client.connect-timeout", "42s",
+                        "hive.metastore.thrift.client.read-timeout", "42s",
+                        "hive.metastore.thrift.impersonation.enabled", "true"),
+                Map.of(
+                        "hive.metastore-timeout", "42s",
+                        "hive.metastore.impersonation-enabled", "true"));
+    }
 }
diff --git a/...t/java/io/trino/plugin/hive/metastore/thrift/TestingTokenAwareMetastoreClientFactory.java b/...t/java/io/trino/plugin/hive/metastore/thrift/TestingTokenAwareMetastoreClientFactory.java
@@ -38,7 +38,7 @@ public TestingTokenAwareMetastoreClientFactory(Optional<HostAndPort> socksProxy,
 
     public TestingTokenAwareMetastoreClientFactory(Optional<HostAndPort> socksProxy, HostAndPort address, Duration timeout)
     {
-        this.factory = new DefaultThriftMetastoreClientFactory(Optional.empty(), socksProxy, timeout, AUTHENTICATION, "localhost");
+        this.factory = new DefaultThriftMetastoreClientFactory(Optional.empty(), socksProxy, timeout, timeout, AUTHENTICATION, "localhost");
         this.address = requireNonNull(address, "address is null");
     }
 

diff --git a/...ino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergMinioConnectorSmokeTest.java b/...ino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergMinioConnectorSmokeTest.java
@@ -69,7 +69,7 @@ protected QueryRunner createQueryRunner()
                                 .put("iceberg.file-format", format.name())
                                 .put("iceberg.catalog.type", "HIVE_METASTORE")
                                 .put("hive.metastore.uri", "thrift://" + hiveMinioDataLake.getHiveHadoop().getHiveMetastoreEndpoint())
-                                .put("hive.metastore-timeout", "1m") // read timed out sometimes happens with the default timeout
+                                .put("hive.metastore.thrift.client.read-timeout", "1m") // read timed out sometimes happens with the default timeout
                                 .put("fs.hadoop.enabled", "false")
                                 .put("fs.native-s3.enabled", "true")
                                 .put("s3.aws-access-key", MINIO_ACCESS_KEY)

diff --git a/...rino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergAbfsConnectorSmokeTest.java b/...rino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergAbfsConnectorSmokeTest.java
@@ -85,7 +85,7 @@ protected QueryRunner createQueryRunner()
                                 .put("iceberg.file-format", format.name())
                                 .put("iceberg.catalog.type", "HIVE_METASTORE")
                                 .put("hive.metastore.uri", "thrift://" + hiveHadoop.getHiveMetastoreEndpoint())
-                                .put("hive.metastore-timeout", "1m") // read timed out sometimes happens with the default timeout
+                                .put("hive.metastore.thrift.client.read-timeout", "1m") // read timed out sometimes happens with the default timeout
                                 .put("hive.azure.abfs-storage-account", account)
                                 .put("hive.azure.abfs-access-key", accessKey)
                                 .put("iceberg.register-table-procedure.enabled", "true")

diff --git a/.../test/java/io/trino/plugin/iceberg/catalog/hms/TestTrinoHiveCatalogWithHiveMetastore.java b/.../test/java/io/trino/plugin/iceberg/catalog/hms/TestTrinoHiveCatalogWithHiveMetastore.java
@@ -99,7 +99,7 @@ protected TrinoCatalog createTrinoCatalog(boolean useUniqueTableLocations)
         ThriftMetastore thriftMetastore = testingThriftHiveMetastoreBuilder()
                 .thriftMetastoreConfig(new ThriftMetastoreConfig()
                         // Read timed out sometimes happens with the default timeout
-                        .setMetastoreTimeout(new Duration(1, MINUTES)))
+                        .setReadTimeout(new Duration(1, MINUTES)))
                 .metastoreClient(dataLake.getHiveHadoop().getHiveMetastoreEndpoint())
                 .build();
         CachingHiveMetastore metastore = memoizeMetastore(new BridgingHiveMetastore(thriftMetastore), 1000);

diff --git a/...oduct-tests/src/main/java/io/trino/tests/product/hive/TestHiveMetastoreClientFactory.java b/...oduct-tests/src/main/java/io/trino/tests/product/hive/TestHiveMetastoreClientFactory.java
@@ -34,6 +34,7 @@ public final class TestHiveMetastoreClientFactory
             Optional.empty(),
             Optional.empty(),
             new Duration(10, SECONDS),
+            new Duration(10, SECONDS),
             new NoHiveMetastoreAuthentication(),
             "localhost");