szehon-ho commented on code in PR #6451:
URL: https://github.com/apache/iceberg/pull/6451#discussion_r1063901869
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -263,7 +286,7 @@ protected void doCommit(TableMetadata base, TableMetadata
metadata) {
tableLevelMutex.lock();
HiveLockHeartbeat hiveLockHeartbeat = null;
try {
- lockId = Optional.of(acquireLock());
+ lockId = Optional.ofNullable(acquireLock(agentInfo));
Review Comment:
Here I get an Intellij warning because acquireLock returns long which is
never null , should we keep Optional.of()?
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -699,18 +717,53 @@ private void cleanupMetadataAndUnlock(
} catch (RuntimeException e) {
LOG.error("Failed to cleanup metadata file at {}", metadataLocation, e);
} finally {
- unlock(lockId);
+ unlock(lockId, agentInfo);
tableLevelMutex.unlock();
}
}
- private void unlock(Optional<Long> lockId) {
- if (lockId.isPresent()) {
- try {
- doUnlock(lockId.get());
- } catch (Exception e) {
- LOG.warn("Failed to unlock {}.{}", database, tableName, e);
+ private void unlock(Optional<Long> lockId, String agentInfo) {
+ Long id = null;
+ try {
+ if (!lockId.isPresent()) {
+ // Try to find the lock based on agentInfo. Only works with Hive 2 or
later.
+ if (MetastoreClientVersion.min(MetastoreClientVersion.HIVE_2)) {
+ AtomicReference<Long> foundLockId = new AtomicReference<>();
+ AtomicReference<LockState> state = new AtomicReference<>();
+ if (!findLock(agentInfo, foundLockId, state)) {
+ // No lock found
+ LOG.info("No lock found with {} agentInfo", agentInfo);
+ return;
+ }
+
+ id = foundLockId.get();
+ } else {
+ LOG.warn("Could not find lock with HMSClient {}",
MetastoreClientVersion.current());
+ return;
+ }
+ } else {
+ id = lockId.get();
+ }
+
+ doUnlock(id);
+ } catch (InterruptedException ie) {
+ if (id != null) {
Review Comment:
Thanks for the explanation, makes sense. It does seem a bit risky for
complex handling of interrupt by unlocking again, unless you are very sure
thats what caused the issue (if the first call is interrupted because of
timeout, I'm not sure what the chance is the unlock retry will succeed), what
do you think? Will leave it to you as you have seen the issue. Also fyi
@stevenzwu
##########
docs/configuration.md:
##########
@@ -160,14 +160,20 @@ Here are the catalog properties related to locking. They
are used by some catalo
The following properties from the Hadoop configuration are used by the Hive
Metastore connector.
-| Property | Default | Description
|
-| ------------------------------------- | ---------------- |
----------------------------------------------------------------------------------
|
-| iceberg.hive.client-pool-size | 5 | The size of the
Hive client pool when tracking tables in HMS |
-| iceberg.hive.lock-timeout-ms | 180000 (3 min) | Maximum time in
milliseconds to acquire a lock |
-| iceberg.hive.lock-check-min-wait-ms | 50 | Minimum time in
milliseconds to check back on the status of lock acquisition |
-| iceberg.hive.lock-check-max-wait-ms | 5000 | Maximum time in
milliseconds to check back on the status of lock acquisition |
-
-Note: `iceberg.hive.lock-check-max-wait-ms` should be less than the
[transaction
timeout](https://cwiki.apache.org/confluence/display/Hive/Configuration+Properties#ConfigurationProperties-hive.txn.timeout)
+| Property | Default | Description
|
+|-------------------------------------------|-----------------|------------------------------------------------------------------------------|
+| iceberg.hive.client-pool-size | 5 | The size of
the Hive client pool when tracking tables in HMS |
+| iceberg.hive.lock-timeout-ms | 180000 (3 min) | Maximum time
in milliseconds to acquire a lock |
Review Comment:
I must have missed the earlier discussion, but should
'iceberg.hive.lock-timeout-ms' be
'iceberg.hive.lock-check-timeout' as its used with
iceberg.hive.lock-check-min-wait-ms.min-wait-ms and
iceberg.hive.lock-check-min-wait-ms.max-wait-ms?
I guess the two phases to me would be :
1. Lock Creation request
2. Lock Check (check for successful acquisition of lock)
--
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]
