You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "manisin (via GitHub)" <gi...@apache.org> on 2023/02/03 22:22:11 UTC
[GitHub] [iceberg] manisin opened a new pull request, #6740: Add application identifier for Snowflake JDBC driver
manisin opened a new pull request, #6740:
URL: https://github.com/apache/iceberg/pull/6740
Adding application identifier to the JDBC driver used by snowflake catalog. This helps identify partner applications (iceberg SDK in this case) that connect via JDBC driver. More details about the JDBC parameter could be found here https://docs.snowflake.com/en/user-guide/jdbc-parameters.html#application
--
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] manisin commented on a diff in pull request #6740: Add application identifier for Snowflake JDBC driver
Posted by "manisin (via GitHub)" <gi...@apache.org>.
manisin commented on code in PR #6740:
URL: https://github.com/apache/iceberg/pull/6740#discussion_r1097809645
##########
snowflake/src/main/java/org/apache/iceberg/snowflake/SnowflakeCatalog.java:
##########
@@ -45,6 +45,7 @@ public class SnowflakeCatalog extends BaseMetastoreCatalog
implements Closeable, SupportsNamespaces, Configurable<Object> {
private static final String DEFAULT_CATALOG_NAME = "snowflake_catalog";
private static final String DEFAULT_FILE_IO_IMPL = "org.apache.iceberg.io.ResolvingFileIO";
+ private static final String APP_IDENTIFIER = "iceberg-SDK";
Review Comment:
Agree `iceberg-snowflake-catalog` would be a better name. Switched to that instead.
--
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] manisin commented on a diff in pull request #6740: Add application identifier for Snowflake JDBC driver
Posted by "manisin (via GitHub)" <gi...@apache.org>.
manisin commented on code in PR #6740:
URL: https://github.com/apache/iceberg/pull/6740#discussion_r1097808842
##########
snowflake/src/main/java/org/apache/iceberg/snowflake/SnowflakeCatalog.java:
##########
@@ -109,6 +110,10 @@ public void initialize(String name, Map<String, String> properties) {
+ " JDBC driver to your jars/packages",
cnfe);
}
+
+ // Populate application identifier in jdbc client
+ properties.put("application", APP_IDENTIFIER);
Review Comment:
APP_ID property is not supported by Snowflake's jdbc driver, it requires application. Also as Daniel pointed out, application property does not uniquely point to each catalog usage but broadly identifies the partner app type.
--
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] danielcweeks commented on a diff in pull request #6740: Add application identifier for Snowflake JDBC driver
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #6740:
URL: https://github.com/apache/iceberg/pull/6740#discussion_r1096955753
##########
snowflake/src/main/java/org/apache/iceberg/snowflake/SnowflakeCatalog.java:
##########
@@ -109,6 +110,10 @@ public void initialize(String name, Map<String, String> properties) {
+ " JDBC driver to your jars/packages",
cnfe);
}
+
+ // Populate application identifier in jdbc client
+ properties.put("application", APP_IDENTIFIER);
Review Comment:
I feel like `APP_ID` would be the identifier of where the catalog is being used (e.g. the spark application id) not something specific to this particular catalog 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: 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] danielcweeks commented on a diff in pull request #6740: Add application identifier for Snowflake JDBC driver
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #6740:
URL: https://github.com/apache/iceberg/pull/6740#discussion_r1096956705
##########
snowflake/src/main/java/org/apache/iceberg/snowflake/SnowflakeCatalog.java:
##########
@@ -45,6 +45,7 @@ public class SnowflakeCatalog extends BaseMetastoreCatalog
implements Closeable, SupportsNamespaces, Configurable<Object> {
private static final String DEFAULT_CATALOG_NAME = "snowflake_catalog";
private static final String DEFAULT_FILE_IO_IMPL = "org.apache.iceberg.io.ResolvingFileIO";
+ private static final String APP_IDENTIFIER = "iceberg-SDK";
Review Comment:
I don't feel like this identifier makes a lot of sense. What is the `SDK` in this context? The snowflake catalog isn't an SDK, so I'm not sure what this is intended to convey.
--
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] danielcweeks commented on a diff in pull request #6740: Add application identifier for Snowflake JDBC driver
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #6740:
URL: https://github.com/apache/iceberg/pull/6740#discussion_r1096958848
##########
snowflake/src/main/java/org/apache/iceberg/snowflake/SnowflakeCatalog.java:
##########
@@ -45,6 +45,7 @@ public class SnowflakeCatalog extends BaseMetastoreCatalog
implements Closeable, SupportsNamespaces, Configurable<Object> {
private static final String DEFAULT_CATALOG_NAME = "snowflake_catalog";
private static final String DEFAULT_FILE_IO_IMPL = "org.apache.iceberg.io.ResolvingFileIO";
+ private static final String APP_IDENTIFIER = "iceberg-SDK";
Review Comment:
I feel like sometime more like `iceberg-snowflake-catalog` would be more inline with what is actually connecting to snowflake.
--
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] danielcweeks merged pull request #6740: Add application identifier for Snowflake JDBC driver
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks merged PR #6740:
URL: https://github.com/apache/iceberg/pull/6740
--
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] nastra commented on a diff in pull request #6740: Add application identifier for Snowflake JDBC driver
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #6740:
URL: https://github.com/apache/iceberg/pull/6740#discussion_r1096666180
##########
snowflake/src/main/java/org/apache/iceberg/snowflake/SnowflakeCatalog.java:
##########
@@ -109,6 +110,10 @@ public void initialize(String name, Map<String, String> properties) {
+ " JDBC driver to your jars/packages",
cnfe);
}
+
+ // Populate application identifier in jdbc client
+ properties.put("application", APP_IDENTIFIER);
Review Comment:
we're using `CatalogProperties.APP_ID` in a few other places, would it make sense to use it here as well or does the JDBC client require `application`?
--
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] danielcweeks commented on a diff in pull request #6740: Add application identifier for Snowflake JDBC driver
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #6740:
URL: https://github.com/apache/iceberg/pull/6740#discussion_r1096959777
##########
snowflake/src/main/java/org/apache/iceberg/snowflake/SnowflakeCatalog.java:
##########
@@ -109,6 +110,10 @@ public void initialize(String name, Map<String, String> properties) {
+ " JDBC driver to your jars/packages",
cnfe);
}
+
+ // Populate application identifier in jdbc client
+ properties.put("application", APP_IDENTIFIER);
Review Comment:
You might want to pull the `application` literal into a static final reference and add a link to the docs where it's used in snowflake since it's not a standard jdbc connector property.
--
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