You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "electrum (via GitHub)" <gi...@apache.org> on 2023/03/11 19:00:05 UTC
[GitHub] [iceberg] electrum opened a new pull request, #7081: Core: Allow injecting resources in REST and JDBC catalogs
electrum opened a new pull request, #7081:
URL: https://github.com/apache/iceberg/pull/7081
Allow catalogs to be stateless by injecting the long-lived resources.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7081: Core: Allow injecting resources in REST and JDBC catalogs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7081:
URL: https://github.com/apache/iceberg/pull/7081#discussion_r1133161005
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -127,6 +136,19 @@ public void setFileIOBuilder(Function<Map<String, String>, FileIO> newIOBuilder)
this.ioBuilder = newIOBuilder;
}
+ public void setClientPoolBuilder(
+ Function<Map<String, String>, JdbcClientPool> newClientPoolBuilder) {
+ Preconditions.checkState(
+ null == connections, "Cannot set client pool builder after calling initialize");
+ this.clientPoolBuilder = newClientPoolBuilder;
+ }
+
+ public void setInitializeCatalogTables(boolean initializeCatalogTables) {
Review Comment:
Got it. We should just have a `initialized` flag on the constructor, too.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7081: Core: Allow injecting resources in REST and JDBC catalogs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7081:
URL: https://github.com/apache/iceberg/pull/7081#discussion_r1133140683
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -126,7 +126,7 @@ public RESTSessionCatalog() {
this(config -> HTTPClient.builder(config).uri(config.get(CatalogProperties.URI)).build());
}
- RESTSessionCatalog(Function<Map<String, String>, RESTClient> clientBuilder) {
+ public RESTSessionCatalog(Function<Map<String, String>, RESTClient> clientBuilder) {
Review Comment:
In general, I'm okay with this approach. I think setting these in the constructor is probably the right way to go if we need to do more of this. We should also remove the new setter method if we're opening this up and add the builder here.
I'm also curious why this is needed, since I think that catalogs probably will have their own client and the set of catalogs doesn't really change in Trino. What do you intend to do differently when you can use this?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] electrum commented on a diff in pull request #7081: Core: Allow injecting resources in REST and JDBC catalogs
Posted by "electrum (via GitHub)" <gi...@apache.org>.
electrum commented on code in PR #7081:
URL: https://github.com/apache/iceberg/pull/7081#discussion_r1133142960
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -127,6 +136,19 @@ public void setFileIOBuilder(Function<Map<String, String>, FileIO> newIOBuilder)
this.ioBuilder = newIOBuilder;
}
+ public void setClientPoolBuilder(
+ Function<Map<String, String>, JdbcClientPool> newClientPoolBuilder) {
+ Preconditions.checkState(
+ null == connections, "Cannot set client pool builder after calling initialize");
+ this.clientPoolBuilder = newClientPoolBuilder;
+ }
+
+ public void setInitializeCatalogTables(boolean initializeCatalogTables) {
Review Comment:
Since we create the catalog class for every Trino query, we don’t want to run this repeatedly.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7081: Core: Allow injecting resources in REST and JDBC catalogs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7081:
URL: https://github.com/apache/iceberg/pull/7081#discussion_r1133160958
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -127,6 +136,19 @@ public void setFileIOBuilder(Function<Map<String, String>, FileIO> newIOBuilder)
this.ioBuilder = newIOBuilder;
}
+ public void setClientPoolBuilder(
Review Comment:
Yeah, let's switch over to constructor. I wasn't sure what would work for Trino originally but that's a better pattern.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7081: Core: Allow injecting resources in REST and JDBC catalogs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7081:
URL: https://github.com/apache/iceberg/pull/7081#discussion_r1133161069
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -126,7 +126,7 @@ public RESTSessionCatalog() {
this(config -> HTTPClient.builder(config).uri(config.get(CatalogProperties.URI)).build());
}
- RESTSessionCatalog(Function<Map<String, String>, RESTClient> clientBuilder) {
+ public RESTSessionCatalog(Function<Map<String, String>, RESTClient> clientBuilder) {
Review Comment:
Okay, I think we settled this offline. We can move forward with something better for REST since we already handle sessions for REST and don't need a catalog instance per user.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] electrum closed pull request #7081: Core: Allow injecting resources in REST and JDBC catalogs
Posted by "electrum (via GitHub)" <gi...@apache.org>.
electrum closed pull request #7081: Core: Allow injecting resources in REST and JDBC catalogs
URL: https://github.com/apache/iceberg/pull/7081
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] electrum commented on a diff in pull request #7081: Core: Allow injecting resources in REST and JDBC catalogs
Posted by "electrum (via GitHub)" <gi...@apache.org>.
electrum commented on code in PR #7081:
URL: https://github.com/apache/iceberg/pull/7081#discussion_r1133143088
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -127,6 +136,19 @@ public void setFileIOBuilder(Function<Map<String, String>, FileIO> newIOBuilder)
this.ioBuilder = newIOBuilder;
}
+ public void setClientPoolBuilder(
Review Comment:
Agreed, constructor injection is better. I was trying to follow the existing pattern.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7081: Core: Allow injecting resources in REST and JDBC catalogs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7081:
URL: https://github.com/apache/iceberg/pull/7081#discussion_r1133140387
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -127,6 +136,19 @@ public void setFileIOBuilder(Function<Map<String, String>, FileIO> newIOBuilder)
this.ioBuilder = newIOBuilder;
}
+ public void setClientPoolBuilder(
Review Comment:
The IO builder seemed like a reasonable thing to get in quickly, but it does conflict with the `initialize` pattern. If we are going to inject more resources then we should come up with a standard way to do that rather that creating a bunch of these setter methods.
I think what we should probably do is inject these hooks in the constructor. Would that work for you, @electrum?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] electrum commented on pull request #7081: Core: Allow injecting resources in REST and JDBC catalogs
Posted by "electrum (via GitHub)" <gi...@apache.org>.
electrum commented on PR #7081:
URL: https://github.com/apache/iceberg/pull/7081#issuecomment-1465702218
Replaced by #7087 and #7088
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7081: Core: Allow injecting resources in REST and JDBC catalogs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7081:
URL: https://github.com/apache/iceberg/pull/7081#discussion_r1133140243
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -127,6 +136,19 @@ public void setFileIOBuilder(Function<Map<String, String>, FileIO> newIOBuilder)
this.ioBuilder = newIOBuilder;
}
+ public void setClientPoolBuilder(
+ Function<Map<String, String>, JdbcClientPool> newClientPoolBuilder) {
+ Preconditions.checkState(
+ null == connections, "Cannot set client pool builder after calling initialize");
+ this.clientPoolBuilder = newClientPoolBuilder;
+ }
+
+ public void setInitializeCatalogTables(boolean initializeCatalogTables) {
Review Comment:
Why is this needed?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] electrum commented on a diff in pull request #7081: Core: Allow injecting resources in REST and JDBC catalogs
Posted by "electrum (via GitHub)" <gi...@apache.org>.
electrum commented on code in PR #7081:
URL: https://github.com/apache/iceberg/pull/7081#discussion_r1133143753
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -126,7 +126,7 @@ public RESTSessionCatalog() {
this(config -> HTTPClient.builder(config).uri(config.get(CatalogProperties.URI)).build());
}
- RESTSessionCatalog(Function<Map<String, String>, RESTClient> clientBuilder) {
+ public RESTSessionCatalog(Function<Map<String, String>, RESTClient> clientBuilder) {
Review Comment:
The Trino FileIO wrapper is created per Trino query, since it has context about the user (for example, HDFS with Kerberos impersonation). Thus, we need to create the Iceberg catalog per query as well.
The mismatch is the scope of the services. FileIO has query scope, but the HTTP client has singleton scope.
We need to determine the appropriate scope for everything inside the REST catalog and make it injectable.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org