Upgrade Glue Client to AWS SDK v2 #17866

ShubhamChaurasia · 2023-06-13T01:22:52Z

Description

This PR updates the glue client to AWS SDK v2. It mainly converts the GlueHiveMetastore and TrinoGlueCatalog used by respectively trino-hive and trino-iceberg to v2.

The packages have been relocated from com.amazonaws.services.glue to software.amazon.awssdk.services.glue. Creation of clients and requests have been moved to builder pattern.

v1 version

new DeleteDatabaseRequest().withName(database)

v2 version

DeleteDatabaseRequest.builder().name(database).build()

In v1 the async client extended the sync client and hence had both the sync and async methods. In v2, the async client only has the async methods which return CompletableFuture. In order to use the async method for a sync call, it needs to be blocked using join() or get(). As this is a common use case the handling has been added as a utility to io.trino.plugin.hive.metastore.glue.AwsSdkUtil.
The request handlers are converted to use the ExecutionInterceptor which is used to intercept the request and perform several actions during the lifecycle of the request.
Converting MetricsRequester to MetricsPublisher to collect metrics and store them in GlueMetastoreStats. Some of the metrics have been changed. Below is the result.

v1 version

select "awsclientexecutetime.alltime.avg", "awsclientexecutetime.alltime.count", "awsrequesttime.alltime.avg", "awsrequesttime.alltime.count", "awsclientretrypausetime.alltime.avg", "awsclientretrypausetime.alltime.count", "awsthrottleexceptions.totalcount", "awsrequestcount.totalcount", "awsretrycount.totalcount", "awshttpclientpoolavailablecount", "awshttpclientpoolleasedcount", "awshttpclientpoolpendingcount"  from "trino.plugin.hive.metastore.glue:name=hive,type=gluehivemetastore";

Result:

"awsclientexecutetime.alltime.avg","awsclientexecutetime.alltime.count","awsrequesttime.alltime.avg","awsrequesttime.alltime.count","awsclientretrypausetime.alltime.avg","awsclientretrypausetime.alltime.count","awsthrottleexceptions.totalcount","awsrequestcount.totalcount","awsretrycount.totalcount","awshttpclientpoolavailablecount","awshttpclientpoolleasedcount","awshttpclientpoolpendingcount"
"505.086064688","125.0","425.98159896799996","125.0","NaN","0.0","0","125","0","5","0","0"

v2 version

select "awsservicecallduration.alltime.avg", "awsservicecallduration.alltime.count", "awsapicallduration.alltime.avg", "awsapicallduration.alltime.count", "awsbackoffdelayduration.alltime.avg", "awsbackoffdelayduration.alltime.count", "awsthrottleexceptions.totalcount", "awsrequestcount.totalcount", "awsretrycount.totalcount", "awshttpclientpoolavailablecount", "awshttpclientpoolleasedcount", "awshttpclientpoolpendingcount"  from "trino.plugin.hive.metastore.glue:name=hive,type=gluehivemetastore";

Result:

"awsservicecallduration.alltime.avg","awsservicecallduration.alltime.count","awsapicallduration.alltime.avg","awsapicallduration.alltime.count","awsbackoffdelayduration.alltime.avg","awsbackoffdelayduration.alltime.count","awsthrottleexceptions.totalcount","awsrequestcount.totalcount","awsretrycount.totalcount","awshttpclientpoolavailablecount","awshttpclientpoolleasedcount","awshttpclientpoolpendingcount"
"920.0434782608696","23.0","924.3913043478261","23.0","0.0","23.0","0","23","0","1","0","0"

Co-authored-by: Karan Makhija[email protected]

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

`hive.metastore.glue.pin-client-to-current-region` is deprecated. Current region will be inferred automatically if running on EC2 machine.

cla-bot · 2023-06-13T01:22:54Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

electrum · 2023-06-13T04:59:13Z

I'm not sure why the original code used the async version of the interface, but we can use the synchronous GlueClient in the new version.

electrum · 2023-06-13T05:00:54Z

plugin/trino-hive/pom.xml

+
+        <dependency>
+            <groupId>software.amazon.awssdk</groupId>
+            <artifactId>http-client-spi</artifactId>


We should use apache-client since we don't need or want async behavior. Please see trino-filesystem-s3 to see how we use it with S3.

I added a few details in this comment. trino-hive may not be able to use sync client as mentioned.
Should I convert the one in trino-iceberg(TrinoGlueCatalog) to sync ?

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/AwsSdkUtil.java

electrum · 2023-06-13T05:03:49Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/AwsSdkUtil.java

+    /**
+     * Helper method to handle sync request with async client
+     */
+    public static <Request, Result> Result awsSyncRequest(


This shouldn't be needed if we use GlueClient.

electrum · 2023-06-13T05:06:14Z

...rino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueCatalogIdRequestHandler.java

+import software.amazon.awssdk.services.glue.model.UpdateColumnStatisticsForTableRequest;
+import software.amazon.awssdk.services.glue.model.UpdateDatabaseRequest;
+import software.amazon.awssdk.services.glue.model.UpdatePartitionRequest;
+import software.amazon.awssdk.services.glue.model.UpdateTableRequest;

 import static java.util.Objects.requireNonNull;

 public class GlueCatalogIdRequestHandler


Since we're updating all the code, I think it would make more sense to drop this class and set the catalog ID directly when creating the request objects. @ebyhr thoughts?

That way we will need to pass catalogId (or GlueHiveMetastoreConfig) to all the parts of the code that create a glue request. Currently this is needed only at one place where the client is getting created, and GlueCatalogIdRequestHandler getting injected there. @electrum @ebyhr thoughts ?

Correct, the alternative is to update all of the call sites in GlueHiveMetastore. It looks like most requests of a given type are only made in one place, so the amount of code should be the same (probably fewer lines). The advantage of the GlueCatalogIdRequestHandler approach is that we guarantee that no places are missed, so perhaps we should leave this alone.

It is unfortunate that the Glue SDK doesn't have a common interface or subclass for Glue requests for the catalog ID methods.

right, have kept it as is.

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueClientUtil.java

...in/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueCredentialsProvider.java

...ino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/converter/GlueInputConverter.java

ShubhamChaurasia · 2023-06-13T13:10:40Z

I'm not sure why the original code used the async version of the interface, but we can use the synchronous GlueClient in the new version.

@electrum thanks for the review. I am working on addressing the other comments. On this one, I think the async client originally existed because of the batchUpdatePartitionAsync, batchGetPartitionAsync, batchCreatePartitionAsync methods in GlueHiveMetastore which operate on the batches of partitions (concurrently). I think it is appropriate for them to be async. With sync client we will have to explicitly do these operations in multiple threads. WDYT ?

TrinoGlueCatalog in iceberg module do not have any such requirements yet, and can work with sync client. I can convert that.

cla-bot · 2023-06-15T09:15:16Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

electrum · 2023-06-21T21:23:04Z

That makes sense. Let's change Iceberg to use synchronous, but use async for Hive. I note that the V1 client actually used an executor internally, but with V2 we can use the async client and reduce thread usage (but with higher fixed thread usage and overhead due to Netty).

ShubhamChaurasia · 2023-06-27T12:25:57Z

Sure @electrum, changed glue calls in iceberg module to use sync client.

findinpath · 2023-07-03T11:12:23Z

@ShubhamChaurasia pls rebase on top of master to address the code conflicts.

ShubhamChaurasia · 2023-07-04T06:12:12Z

@findinpath rebased and updated.

This CR updates the glue client to aws sdk v2. It mainly converts the GlueHiveMetastore and TrinoGlueCatalog used by respectively `trino-hive` and `trino-iceberg` to v2. - The packages have been relocated from com.amazonaws.services.glue to software.amazon.awssdk.services.glue. Creation of clients and requests have been moved to builder pattern. v1 version ``` new DeleteDatabaseRequest().withName(database) ``` v2 version ``` DeleteDatabaseRequest.builder().name(database).build() ``` - In v1 the async client extended the sync client and hence had both the sync and async methods. In v2, the async client only has the async methods which return CompletableFuture. In order to use the async method for a sync call, it needs to be blocked using join() or get(). As this is a common use case the handling has been added as a utility to io.trino.plugin.hive.metastore.glue.AwsSdkUtil. - The request handlers are converted to use the `ExecutionInterceptor` which is used to intercept the request and perform several actions during the lifecycle of the request. - Converting MetricsRequester to MetricsPublisher to collect metrics and store them in GlueMetastoreStats. Some of the metrics have been changed. Below is the result. v1 version ``` select "awsclientexecutetime.alltime.avg", "awsclientexecutetime.alltime.count", "awsrequesttime.alltime.avg", "awsrequesttime.alltime.count", "awsclientretrypausetime.alltime.avg", "awsclientretrypausetime.alltime.count", "awsthrottleexceptions.totalcount", "awsrequestcount.totalcount", "awsretrycount.totalcount", "awshttpclientpoolavailablecount", "awshttpclientpoolleasedcount", "awshttpclientpoolpendingcount" from "trino.plugin.hive.metastore.glue:name=hive,type=gluehivemetastore"; Result: "awsclientexecutetime.alltime.avg","awsclientexecutetime.alltime.count","awsrequesttime.alltime.avg","awsrequesttime.alltime.count","awsclientretrypausetime.alltime.avg","awsclientretrypausetime.alltime.count","awsthrottleexceptions.totalcount","awsrequestcount.totalcount","awsretrycount.totalcount","awshttpclientpoolavailablecount","awshttpclientpoolleasedcount","awshttpclientpoolpendingcount" "505.086064688","125.0","425.98159896799996","125.0","NaN","0.0","0","125","0","5","0","0" ``` v2 version ``` select "awsservicecallduration.alltime.avg", "awsservicecallduration.alltime.count", "awsapicallduration.alltime.avg", "awsapicallduration.alltime.count", "awsbackoffdelayduration.alltime.avg", "awsbackoffdelayduration.alltime.count", "awsthrottleexceptions.totalcount", "awsrequestcount.totalcount", "awsretrycount.totalcount", "awshttpclientpoolavailablecount", "awshttpclientpoolleasedcount", "awshttpclientpoolpendingcount" from "trino.plugin.hive.metastore.glue:name=hive,type=gluehivemetastore"; Result: "awsservicecallduration.alltime.avg","awsservicecallduration.alltime.count","awsapicallduration.alltime.avg","awsapicallduration.alltime.count","awsbackoffdelayduration.alltime.avg","awsbackoffdelayduration.alltime.count","awsthrottleexceptions.totalcount","awsrequestcount.totalcount","awsretrycount.totalcount","awshttpclientpoolavailablecount","awshttpclientpoolleasedcount","awshttpclientpoolpendingcount" "920.0434782608696","23.0","924.3913043478261","23.0","0.0","23.0","0","23","0","1","0","0" ``` Co-authored-by: Karan Makhija<[email protected]>

…client-to-current-region This commit addresses review comments and includes following changes - 1. Uses paginator APIs provided by SDK V2 to avoid handling pagination externally 2. Deprecates "hive.metastore.glue.pin-client-to-current-region" as the SDK will automatically try to determine the region if the code is running in EC2 machine.

…Metastore

ShubhamChaurasia · 2023-07-18T06:57:03Z

@electrum @findinpath rebased again, could you please check ?

github-actions · 2024-01-15T17:03:21Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

mosabua · 2024-01-15T18:46:50Z

👋 @ShubhamChaurasia @findinpath @electrum .. could you please collaborate and figure out if this PR and approach is still valid and should be implemented.

We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.

findinpath · 2024-01-15T19:23:16Z

@ShubhamChaurasia pls rebase. We should be now in good shape to review the changes to eventually land this PR.

ShubhamChaurasia · 2024-01-16T13:46:08Z

@mosabua @findinpath thanks for letting me know, will give rebase a try.

github-actions · 2024-02-07T17:02:48Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

findinpath · 2024-02-12T13:27:24Z

@ShubhamChaurasia do you still plan to continue with the work needed to land this PR?

github-actions · 2024-03-05T17:11:01Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

github-actions · 2024-03-27T17:02:52Z

Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time.

mosabua · 2024-05-17T22:09:44Z

This PR is made redundant by the merged update with much more improvements in #20657

github-actions bot added delta-lake Delta Lake connector hive Hive connector iceberg Iceberg connector tests:hive labels Jun 13, 2023

electrum reviewed Jun 13, 2023

View reviewed changes

ShubhamChaurasia force-pushed the aws-sdk-v2-upgrade/glue branch from 1c56b7c to a654ea1 Compare June 27, 2023 06:37

cla-bot bot added the cla-signed label Jun 27, 2023

ShubhamChaurasia force-pushed the aws-sdk-v2-upgrade/glue branch 2 times, most recently from cc68a9a to 4343d86 Compare July 4, 2023 03:32

ShubhamChaurasia requested review from electrum and findinpath July 4, 2023 06:12

ankushChatterjee mentioned this pull request Jul 7, 2023

Support S3 Select on native readers #17522

Closed

ShubhamChaurasia added 5 commits July 18, 2023 10:21

Address review comments - change to Glue sync client for iceberg module

a754210

Adds provider for GlueClient, fixes test - TestIcebergPlugin#testGlue…

bd6975c

…Metastore

Dependency fixes after rebase

b227e82

ShubhamChaurasia force-pushed the aws-sdk-v2-upgrade/glue branch from 4343d86 to b227e82 Compare July 18, 2023 05:25

github-actions bot added the stale label Jan 15, 2024

github-actions bot removed the stale label Jan 16, 2024

github-actions bot added the stale label Feb 7, 2024

findinpath mentioned this pull request Feb 12, 2024

Glue v2 #20657

Merged

github-actions bot removed the stale label Feb 12, 2024

github-actions bot added the stale label Mar 5, 2024

github-actions bot closed this Mar 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade Glue Client to AWS SDK v2 #17866

Upgrade Glue Client to AWS SDK v2 #17866

ShubhamChaurasia commented Jun 13, 2023 •

edited

Loading

cla-bot bot commented Jun 13, 2023

electrum commented Jun 13, 2023

electrum Jun 13, 2023

ShubhamChaurasia Jun 15, 2023

electrum Jun 13, 2023

electrum Jun 13, 2023

ShubhamChaurasia Jun 15, 2023

electrum Jun 21, 2023 •

edited

Loading

ShubhamChaurasia Jun 27, 2023

ShubhamChaurasia commented Jun 13, 2023

cla-bot bot commented Jun 15, 2023

electrum commented Jun 21, 2023

ShubhamChaurasia commented Jun 27, 2023

findinpath commented Jul 3, 2023

ShubhamChaurasia commented Jul 4, 2023

ShubhamChaurasia commented Jul 18, 2023

github-actions bot commented Jan 15, 2024

mosabua commented Jan 15, 2024

findinpath commented Jan 15, 2024

ShubhamChaurasia commented Jan 16, 2024

github-actions bot commented Feb 7, 2024

findinpath commented Feb 12, 2024

github-actions bot commented Mar 5, 2024

github-actions bot commented Mar 27, 2024

mosabua commented May 17, 2024

Upgrade Glue Client to AWS SDK v2 #17866

Upgrade Glue Client to AWS SDK v2 #17866

Conversation

ShubhamChaurasia commented Jun 13, 2023 • edited Loading

Description

Release notes

cla-bot bot commented Jun 13, 2023

electrum commented Jun 13, 2023

electrum Jun 13, 2023

Choose a reason for hiding this comment

ShubhamChaurasia Jun 15, 2023

Choose a reason for hiding this comment

electrum Jun 13, 2023

Choose a reason for hiding this comment

electrum Jun 13, 2023

Choose a reason for hiding this comment

ShubhamChaurasia Jun 15, 2023

Choose a reason for hiding this comment

electrum Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

ShubhamChaurasia Jun 27, 2023

Choose a reason for hiding this comment

ShubhamChaurasia commented Jun 13, 2023

cla-bot bot commented Jun 15, 2023

electrum commented Jun 21, 2023

ShubhamChaurasia commented Jun 27, 2023

findinpath commented Jul 3, 2023

ShubhamChaurasia commented Jul 4, 2023

ShubhamChaurasia commented Jul 18, 2023

github-actions bot commented Jan 15, 2024

mosabua commented Jan 15, 2024

findinpath commented Jan 15, 2024

ShubhamChaurasia commented Jan 16, 2024

github-actions bot commented Feb 7, 2024

findinpath commented Feb 12, 2024

github-actions bot commented Mar 5, 2024

github-actions bot commented Mar 27, 2024

mosabua commented May 17, 2024

ShubhamChaurasia commented Jun 13, 2023 •

edited

Loading

electrum Jun 21, 2023 •

edited

Loading