pvary commented on code in PR #6570:
URL: https://github.com/apache/iceberg/pull/6570#discussion_r1171238063
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -572,8 +595,39 @@ private static boolean hiveEngineEnabled(TableMetadata
metadata, Configuration c
ConfigProperties.ENGINE_HIVE_ENABLED,
TableProperties.ENGINE_HIVE_ENABLED_DEFAULT);
}
+ /**
+ * Returns if the hive locking should be enabled on the table, or not.
+ *
+ * <p>The decision is made like this:
+ *
+ * <ol>
+ * <li>Table property value {@link TableProperties#HIVE_LOCK_ENABLED}
+ * <li>If the table property is not set then check the hive-site.xml
property value {@link
+ * ConfigProperties#LOCK_HIVE_ENABLED}
+ * <li>If none of the above is enabled then use the default value {@link
+ * TableProperties#HIVE_LOCK_ENABLED_DEFAULT}
+ * </ol>
+ *
+ * @param metadata Table metadata to use
+ * @param conf The hive configuration to use
+ * @return if the hive engine related values should be enabled or not
+ */
+ private static boolean hiveLockEnabled(TableMetadata metadata, Configuration
conf) {
Review Comment:
> Remove catalog config altogether
Then it is not possible to handle 2/3 use-cases (We either have to set the
flag for every table, or rely on the default provided by the Iceberg source
code - I think it is more useful for the user to provide an override
possibility on catalog level too). I am open to debate the usefulness of this
cases, but if we want to support them, then we need 2 levels of configuration.
> Have Client A throw exception in this case (as I was mentioning, this may
be better)
We can throw an exception if the Table level and the Catalog level config is
contradictory. In this case every table used by a Catalog instance should have
the same locking configuration if it is set at least for one of the tables used.
If we decide on this then I agree with you that there is no need to have a
Catalog level configuration, as it is just for throwing exceptions in some
cases.
I am starting to feel that the main difference is:
- What do we consider more important:
- Configuration flexibility: If the user wants to set all of the tables
to use the new Locking mechanism, they should not need to go to every one of
the tables and alter them one-by-one. A global catalog configuration should be
enough in these cases. For me the question was how to handle when we would like
to use a table differently than the general one and the table level config is
proposed to do that.
- Preventing wrong configurations: With the "only Table level config"
solution we can be sure that if every writer uses which uses a code version
which contains these changes will not have a writing conflict. BTW this can be
archived even with the previous proposal if all of the tables are altered
one-by-one to set the Table level config.
--
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]
