You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "danielcweeks (via GitHub)" <gi...@apache.org> on 2023/05/15 17:00:47 UTC
[GitHub] [iceberg] danielcweeks commented on a diff in pull request #7580: Add unique JDBC application identifier and user agent header
danielcweeks commented on code in PR #7580:
URL: https://github.com/apache/iceberg/pull/7580#discussion_r1194090174
##########
snowflake/src/main/java/org/apache/iceberg/snowflake/SnowflakeCatalog.java:
##########
@@ -48,8 +51,12 @@ public class SnowflakeCatalog extends BaseMetastoreCatalog
// Specifies the name of a Snowflake's partner application to connect through JDBC.
// https://docs.snowflake.com/en/user-guide/jdbc-parameters.html#application
private static final String JDBC_APPLICATION_PROPERTY = "application";
+ // Add a suffix to user agent header for the web requests made by the jdbc driver.
+ private static final String JDBC_USER_AGENT_SUFFIX_PROPERTY = "user_agent_suffix";
private static final String APP_IDENTIFIER = "iceberg-snowflake-catalog";
-
+ // Specifies the length of unique id for each catalog initialized session. Should be less than 32
Review Comment:
Can you provide an explanation as to why there is a limit? Also, I see below that you strip out the `-` characters, which would result in a 32 char string, so why further reduce it to 20 chars which compromises the uniqueness guarantees?
##########
snowflake/src/main/java/org/apache/iceberg/snowflake/SnowflakeCatalog.java:
##########
@@ -48,8 +51,12 @@ public class SnowflakeCatalog extends BaseMetastoreCatalog
// Specifies the name of a Snowflake's partner application to connect through JDBC.
// https://docs.snowflake.com/en/user-guide/jdbc-parameters.html#application
private static final String JDBC_APPLICATION_PROPERTY = "application";
+ // Add a suffix to user agent header for the web requests made by the jdbc driver.
+ private static final String JDBC_USER_AGENT_SUFFIX_PROPERTY = "user_agent_suffix";
Review Comment:
Can you document that this is a property specifically used by the snowflake jdbc driver? I assume that's the case, but don't see any reference to its use.
##########
snowflake/src/main/java/org/apache/iceberg/snowflake/SnowflakeCatalog.java:
##########
@@ -114,8 +121,14 @@ public void initialize(String name, Map<String, String> properties) {
cnfe);
}
+ String uniqueId = UUID.randomUUID().toString().replace("-", "").substring(0, UNIQUE_ID_LENGTH);
+ String uniqueAppIdentifier = APP_IDENTIFIER + "_" + uniqueId;
+ String icebergVersion = IcebergBuild.fullVersion() + IcebergBuild.gitCommitShortId();
Review Comment:
I'm not sure this is going to produce something nicely formatted. For example:
```scala
> IcebergBuild.fullVersion() + IcebergBuild.gitCommitShortId()
res1: String = Apache Iceberg 1.2.1 (commit 4e2cdccd7453603af42a090fc5530f2bd20cf1be)4e2cdcc
```
`IcebergBuild.fullVersion()` already includes the commit id, so you probably don't need the short version.
--
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