-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Glue v2 #20657
Conversation
898ec70
to
db30322
Compare
FYI there is also another PR tackling the AWS Glue library update #17866 |
@findinpath this is closer to a full rewrite than an upgrade, so this would invalidate that other PR |
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
3689143
to
7a088a5
Compare
8240afc
to
4c7100c
Compare
...o-delta-lake/src/main/java/io/trino/plugin/deltalake/metastore/DeltaLakeMetastoreModule.java
Outdated
Show resolved
Hide resolved
plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/StorageFormat.java
Show resolved
Hide resolved
plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueContext.java
Outdated
Show resolved
Hide resolved
plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueContext.java
Outdated
Show resolved
Hide resolved
...in/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueHiveMetastoreConfig.java
Outdated
Show resolved
Hide resolved
plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueMetastoreModule.java
Outdated
Show resolved
Hide resolved
plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHivePlugin.java
Outdated
Show resolved
Hide resolved
plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueHiveMetastore.java
Outdated
Show resolved
Hide resolved
Leave some implementation in place, so changes to v2 produce readable diffs
@@ -199,7 +199,7 @@ public GlueHiveMetastore( | |||
config.getPartitionSegments(), | |||
config.isAssumeCanonicalPartitionKeys(), | |||
visibleTableKinds, | |||
taskWrapping(newThreadPerTaskExecutor(Thread.ofVirtual().name("glue-", 0L).factory()))); | |||
newFixedThreadPool(config.getThreads(), Thread.ofPlatform().name("glue-", 0L).factory())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switch Glue metastore to platform threads
why this particular change?
(i like it because it makes it possible to detect thread leaks. the API used by #21470 doesn't report virtual threads currently, but i assume this is not the reason)
jmx metrics |
@sudohainguyen |
maybe it's not exported because it's not public? i don't know how this works. you will need to experiment a bit |
yeah I tried 😅 but not able to get metrics from |
"hive.metastore.glue.read-statistics-threads", | ||
"hive.metastore.glue.write-statistics-threads", | ||
"hive.metastore.glue.proxy-api-id", | ||
"hive.metastore.glue.aws-credentials-provider", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR removed the ability to specify a custom AWS credentials provider for Glue v2. Is that intentional @dain @findepi? Custom provider is useful for working around some corner cases, like #15267, which still applies to AWS SDK v2. Can we get a replacement for this similar to what's being done for S3 in #22162 and #22163?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #22425 to address this.
Description
New metstore implementation based in the Glue v2 apis.
New caching caching system built directly into Glue which supports caching of the full objects returned during listing operations.
Release notes
(X) Release notes are required, with the following suggested text: