amogh-jahagirdar commented on code in PR #13594:
URL: https://github.com/apache/iceberg/pull/13594#discussion_r2222566699
##########
docs/docs/configuration.md:
##########
@@ -178,6 +178,7 @@ The HMS table locking is a 2-step process:
| iceberg.hive.metadata-refresh-max-retries | 2 | Maximum number
of retries when the metadata file is missing |
| iceberg.hive.table-level-lock-evict-ms | 600000 (10 min) | The timeout
for the JVM table lock is |
| iceberg.engine.hive.lock-enabled | true | Use HMS locks
to ensure atomicity of commits |
+| iceberg.glue.metadata-refresh-max-retries | 20 | Maximum number
of retries when the metadata file is missing |
Review Comment:
Btw just to make sure we're on the same page about addressing the right
problem, the code path that's being changed here will actually _stop_ retrying
when the metadata file is missing; the retries address any other kind of
failure.
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -77,6 +78,10 @@ class GlueTableOperations extends
BaseMetastoreTableOperations {
private final Object hadoopConf;
private final LockManager lockManager;
private FileIO fileIO;
+ private final int metadataRefreshMaxRetries;
+ private static final String GLUE_ICEBERG_METADATA_REFRESH_MAX_RETRIES =
+ "iceberg.glue.metadata-refresh-max-retries";
+ private static final int GLUE_ICEBERG_METADATA_REFRESH_MAX_RETRIES_DEFAULT =
20;
Review Comment:
If we were to do this, I'd say this should probably be something like a Glue
catalog property defined in `AwsProperties` instead of through Hadoop conf.
But I still think we should instead try to avoid another configuration here
and just use the ability to plug in your own Glue client with your desired
behavior as the way to address this.
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java:
##########
@@ -172,6 +172,10 @@ void initialize(
LockManager lock,
Map<String, String> catalogProps) {
this.catalogProperties = catalogProps;
+ if (hadoopConf == null) {
+ LOG.warn("No Hadoop Configuration was set, using the default environment
Configuration");
+ this.hadoopConf = new Configuration();
+ }
Review Comment:
See my comment below about how if we were to do this, I don't think it
should be through hadoop conf
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
