kbendick commented on a change in pull request #3379:
URL: https://github.com/apache/iceberg/pull/3379#discussion_r737790307
##########
File path:
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
##########
@@ -161,6 +164,8 @@ protected HiveTableOperations(Configuration conf,
ClientPool metaClients, FileIO
conf.getLong(HIVE_LOCK_CHECK_MIN_WAIT_MS,
HIVE_LOCK_CHECK_MIN_WAIT_MS_DEFAULT);
this.lockCheckMaxWaitTime =
conf.getLong(HIVE_LOCK_CHECK_MAX_WAIT_MS,
HIVE_LOCK_CHECK_MAX_WAIT_MS_DEFAULT);
+ this.metadataRefreshMaxRetries =
conf.getInt(HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES,
+ HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES_DEFAULT);
Review comment:
Nit: Can you put the entire `conf.getInt` call on the next line, and
line it up with the ones above it? It looks over-indented to me given the
surrounding statements.
I don't think it's actually over-indented, but given that it pops out
strangely, if you can fit the whole call on the next line, that would be
preferable.
If it can't fit on one line, then you can just leave it as is. It's not in
violation of the style guide, it just looks like it to me personally on first
glance because it's not lined up like the other calls surrounding it. Just a
nit and not a huge issue.
```java
this.metadataRefreshMaxRetries =
conf.getInt(HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES,
HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES_DEFAULT);
```
##########
File path:
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
##########
@@ -87,9 +87,11 @@
private static final String HIVE_LOCK_CHECK_MIN_WAIT_MS =
"iceberg.hive.lock-check-min-wait-ms";
private static final String HIVE_LOCK_CHECK_MAX_WAIT_MS =
"iceberg.hive.lock-check-max-wait-ms";
private static final String HIVE_TABLE_LEVEL_LOCK_EVICT_MS =
"iceberg.hive.table-level-lock-evict-ms";
+ private static final String HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES =
"iceberg.hive.metadata-refresh-max-retries";
private static final long HIVE_ACQUIRE_LOCK_TIMEOUT_MS_DEFAULT = 3 * 60 *
1000; // 3 minutes
private static final long HIVE_LOCK_CHECK_MIN_WAIT_MS_DEFAULT = 50; // 50
milliseconds
private static final long HIVE_LOCK_CHECK_MAX_WAIT_MS_DEFAULT = 5 * 1000; //
5 seconds
+ private static final int HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES_DEFAULT =
2;
Review comment:
Yeah that works for me. If there wasn't the existing signature, I'd say
keep it as a long because it looks nice in the file. But looking nice isn't
really reason enough to need it to be a wider type. And if INT_MAX retries
isn't sufficient, probably there's bigger problems going on.
##########
File path:
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableBaseTest.java
##########
@@ -79,7 +79,7 @@ protected static String getTableLocation(String tableName) {
return getTableLocationPath(tableName).toString();
}
- private static String metadataLocation(String tableName) {
+ protected static String metadataLocation(String tableName) {
Review comment:
Why did this need to change from `private`?
Oh for the test below. And this is only a test class 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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
