You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2020/01/15 15:15:12 UTC
[GitHub] [incubator-iceberg] waterlx opened a new pull request #736: Add
timeout for acquiring locks in HiveTableOperations
waterlx opened a new pull request #736: Add timeout for acquiring locks in HiveTableOperations
URL: https://github.com/apache/incubator-iceberg/pull/736
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#736: Add timeout for acquiring locks in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #736: Add timeout for acquiring locks in HiveTableOperations
URL: https://github.com/apache/incubator-iceberg/pull/736#discussion_r367010124
##########
File path: hive/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
##########
@@ -249,13 +257,32 @@ private long acquireLock() throws UnknownHostException, TException, InterruptedE
LockResponse lockResponse = metaClients.run(client -> client.lock(lockRequest));
LockState state = lockResponse.getState();
long lockId = lockResponse.getLockid();
- //TODO add timeout
+
+ final long start = System.currentTimeMillis();
+ long duration = 0;
+ boolean timeout = false;
while (state.equals(LockState.WAITING)) {
lockResponse = metaClients.run(client -> client.checkLock(lockId));
state = lockResponse.getState();
+
+ // check timeout
+ duration = System.currentTimeMillis() - start;
+ if (duration > lockAcquireTimeout) {
+ timeout = true;
+ break;
+ }
+
Thread.sleep(50);
}
+ // timeout and do not have lock acquired
+ if (timeout && !state.equals(LockState.ACQUIRED)) {
+ throw new CommitFailedException(String.format("Timeout when acquiring the lock on %s.%s, " +
+ "have waited for %s ms, more than %s ms set by %s",
Review comment:
How about "Timed out after %s ms waiting for lock on %s.%s"? I don't think we need to specify the config property or the timeout threshold.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on issue #736: Add timeout for
acquiring locks in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
rdblue commented on issue #736: Add timeout for acquiring locks in HiveTableOperations
URL: https://github.com/apache/incubator-iceberg/pull/736#issuecomment-575250665
Thanks @waterlx! Looks good so I merged it.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] waterlx commented on issue #736: Add timeout
for acquiring locks in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
waterlx commented on issue #736: Add timeout for acquiring locks in HiveTableOperations
URL: https://github.com/apache/incubator-iceberg/pull/736#issuecomment-575245930
@rdblue Thanks for your review and comments! All are addressed. Would you please review it at your most convenience.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] waterlx commented on issue #736: Add timeout
for acquiring locks in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
waterlx commented on issue #736: Add timeout for acquiring locks in HiveTableOperations
URL: https://github.com/apache/incubator-iceberg/pull/736#issuecomment-574706260
@aokolnychyi Could you please review it at your most convenience?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#736: Add timeout for acquiring locks in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #736: Add timeout for acquiring locks in HiveTableOperations
URL: https://github.com/apache/incubator-iceberg/pull/736#discussion_r367007886
##########
File path: hive/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
##########
@@ -65,18 +65,26 @@
public class HiveTableOperations extends BaseMetastoreTableOperations {
private static final Logger LOG = LoggerFactory.getLogger(HiveTableOperations.class);
+ private static final String HIVE_ACQUIRE_LOCK_STATE_TIMEOUT_MS = "hive.lock.acquire-timeout-ms";
+ private static final long HIVE_ACQUIRE_LOCK_STATE_TIMEOUT_MS_DEFAULT = 3 * 60 * 1000; // 3 minutes
+
private final HiveClientPool metaClients;
private final String database;
private final String tableName;
private final Configuration conf;
+ private final long lockAcquireTimeout;
Review comment:
There's no need for this field to be in a separate group.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#736: Add timeout for acquiring locks in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #736: Add timeout for acquiring locks in HiveTableOperations
URL: https://github.com/apache/incubator-iceberg/pull/736#discussion_r367009330
##########
File path: hive/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
##########
@@ -249,13 +257,32 @@ private long acquireLock() throws UnknownHostException, TException, InterruptedE
LockResponse lockResponse = metaClients.run(client -> client.lock(lockRequest));
LockState state = lockResponse.getState();
long lockId = lockResponse.getLockid();
- //TODO add timeout
+
+ final long start = System.currentTimeMillis();
+ long duration = 0;
+ boolean timeout = false;
while (state.equals(LockState.WAITING)) {
lockResponse = metaClients.run(client -> client.checkLock(lockId));
state = lockResponse.getState();
+
+ // check timeout
+ duration = System.currentTimeMillis() - start;
+ if (duration > lockAcquireTimeout) {
+ timeout = true;
+ break;
Review comment:
Instead of break, can we use `timeout` in the `while` condition?
```java
while (!timeout && state.equals(LockState.WAITING)) { ... }
```
As long as we are setting `timeout`, I think it is nice to use it to avoid control flow breaks. We would also need to move `Thread.sleep(50)` into an `else`.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#736: Add timeout for acquiring locks in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #736: Add timeout for acquiring locks in HiveTableOperations
URL: https://github.com/apache/incubator-iceberg/pull/736#discussion_r367008009
##########
File path: hive/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
##########
@@ -65,18 +65,26 @@
public class HiveTableOperations extends BaseMetastoreTableOperations {
private static final Logger LOG = LoggerFactory.getLogger(HiveTableOperations.class);
+ private static final String HIVE_ACQUIRE_LOCK_STATE_TIMEOUT_MS = "hive.lock.acquire-timeout-ms";
+ private static final long HIVE_ACQUIRE_LOCK_STATE_TIMEOUT_MS_DEFAULT = 3 * 60 * 1000; // 3 minutes
Review comment:
Nit: no need to align variable names.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#736: Add timeout for acquiring locks in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #736: Add timeout for acquiring locks in HiveTableOperations
URL: https://github.com/apache/incubator-iceberg/pull/736#discussion_r367008232
##########
File path: hive/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
##########
@@ -65,18 +65,26 @@
public class HiveTableOperations extends BaseMetastoreTableOperations {
private static final Logger LOG = LoggerFactory.getLogger(HiveTableOperations.class);
+ private static final String HIVE_ACQUIRE_LOCK_STATE_TIMEOUT_MS = "hive.lock.acquire-timeout-ms";
+ private static final long HIVE_ACQUIRE_LOCK_STATE_TIMEOUT_MS_DEFAULT = 3 * 60 * 1000; // 3 minutes
+
private final HiveClientPool metaClients;
private final String database;
private final String tableName;
private final Configuration conf;
+ private final long lockAcquireTimeout;
+
private FileIO fileIO;
protected HiveTableOperations(Configuration conf, HiveClientPool metaClients, String database, String table) {
this.conf = conf;
this.metaClients = metaClients;
this.database = database;
this.tableName = table;
+
Review comment:
Same here, I don't think we need to separate the lock timeout setting from the other fields that are getting initialized.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue merged pull request #736: Add timeout
for acquiring locks in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #736: Add timeout for acquiring locks in HiveTableOperations
URL: https://github.com/apache/incubator-iceberg/pull/736
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#736: Add timeout for acquiring locks in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #736: Add timeout for acquiring locks in HiveTableOperations
URL: https://github.com/apache/incubator-iceberg/pull/736#discussion_r367007221
##########
File path: hive/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
##########
@@ -65,18 +65,26 @@
public class HiveTableOperations extends BaseMetastoreTableOperations {
private static final Logger LOG = LoggerFactory.getLogger(HiveTableOperations.class);
+ private static final String HIVE_ACQUIRE_LOCK_STATE_TIMEOUT_MS = "hive.lock.acquire-timeout-ms";
Review comment:
How about `iceberg.hive.lock-timeout-ms` instead? It isn't a Hive option, it is an Iceberg option for working with Hive; that's why I think we should use the `iceberg` namespace.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org