rdblue commented on a change in pull request #1873:
URL: https://github.com/apache/iceberg/pull/1873#discussion_r538893914
##########
File path:
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
##########
@@ -294,32 +313,50 @@ private long acquireLock() throws UnknownHostException,
TException, InterruptedE
System.getProperty("user.name"),
InetAddress.getLocalHost().getHostName());
LockResponse lockResponse = metaClients.run(client ->
client.lock(lockRequest));
- LockState state = lockResponse.getState();
+ AtomicReference<LockState> state = new
AtomicReference<>(lockResponse.getState());
long lockId = lockResponse.getLockid();
final long start = System.currentTimeMillis();
long duration = 0;
boolean timeout = false;
- while (!timeout && state.equals(LockState.WAITING)) {
- lockResponse = metaClients.run(client -> client.checkLock(lockId));
- state = lockResponse.getState();
- // check timeout
- duration = System.currentTimeMillis() - start;
- if (duration > lockAcquireTimeout) {
+ if (state.get().equals(LockState.WAITING)) {
+ try {
+ Tasks.foreach(lockId)
+ .retry(Integer.MAX_VALUE - 100) // Endless retries bound by
timeouts. Tasks.retry adds 1 for "first try".
+ .exponentialBackoff(
+ lockCheckMinWaitTime,
+ lockCheckMaxWaitTime,
+ lockAcquireTimeout,
+ 1.5)
+ .throwFailureWhenFinished()
+ .onlyRetryOn(WaitingForLockException.class)
+ .run(id -> {
+ try {
+ LockResponse response = metaClients.run(client ->
client.checkLock(id));
+ LockState newState = response.getState();
+ state.set(newState);
+ if (newState.equals(LockState.WAITING)) {
+ throw new WaitingForLockException("Waiting for lock.");
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new RuntimeException("Interrupted while checking lock
status.", e);
Review comment:
I don't think it is necessary to throw `RuntimeException` here. If this
doesn't throw `WaitingForLockException` then it will exit and move on. Since
timeout is not set, it would hit the check for whether the lock was acquired
and fail, resulting in the `CommitFailedException`.
I think that's a fairly reasonable way to handle an interrupt without
wrapping it in a `RuntimeException`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
