[
https://issues.apache.org/jira/browse/HBASE-8205?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13616473#comment-13616473
]
Jonathan Hsieh commented on HBASE-8205:
---------------------------------------
High order bit -- looks good but have questions about lock semantics in some
places. Please consider improving the javadoc (HBASE-8177) so that the table
locks are easier to use/review.
This seems out of place for the generic lock interface. Can you explain why
this changed?
{code}
*/
- public void reapAllLocks() throws IOException;
+ public void reapAllWriteLocks() throws IOException;
{code}
javadoc: this is both the read and write aspects of the table lock right?
{code}
/**
+ * Visits all table locks, and lock attempts with the given callback
+ * MetadataHandler.
+ * @param handler the metadata handler to call
+ * @throws IOException If there is an unrecoverable error
+ */
+ public abstract void visitLocks(MetadataHandler handler) throws IOException;
+
+ /**
+ * Force releases table locks that have been held longer than
"hbase.table.lock.expire.ms".
+ * The behavior of the lock holders still thinking that they have the lock
is undefined.
+ * @throws IOException If there is an unrecoverable error
+ */
+ public abstract void reapExpiredLocks() throws IOException;
+
{code}
javadoc here says the value must not be null[1]. Why is this
lock.writeLock(null) ok?
{code}
- for (String tableName : tableNames) {
+ @Override
+ public void reapExpiredLocks() throws IOException {
+ //get the table names
+ try {
+ for (String tableName : getTableNames()) {
String tableLockZNode = ZKUtil.joinZNode(zkWatcher.tableLockZNode,
tableName);
ZKInterProcessReadWriteLock lock = new ZKInterProcessReadWriteLock(
zkWatcher, tableLockZNode, null);
- lock.writeLock(null).reapAllLocks();
+ lock.writeLock(null).reapExpiredLocks(lockExpireTimeoutMs);
{code}
Should this happen before or after the other repairs? At the least it should
be documented so that the user could pick and choose which repair to do. It
might be better to have it so that if no other repair features are flagged this
is the only thing run (so we don't pound the fs gathering info).
{code}
@@ -455,6 +457,8 @@ public class HBaseFsck extends Configured implements Tool {
offlineReferenceFileRepair();
+ checkAndFixTableLocks();
+
{code}
comment nit: double negatives are complicated
{code}
+ hbck = doFsck(conf, false);
+ assertNoErrors(hbck); //should not have expired no problems
+
{code}
nit: changed to more explicit code for readability?
{code}
@Override
public int compare(String zNode1, String zNode2) {
- int seq1 = getChildSequenceId(zNode1);
- int seq2 = getChildSequenceId(zNode2);
- return seq1 - seq2;
+ long seq1 = getChildSequenceId(zNode1);
+ long seq2 = getChildSequenceId(zNode2);
+ if (seq1 == seq2) {
+ return 0;
+ } else {
+ return seq1 < seq2 ? -1 : 1;
+ }
}
}
{code}
Need to explain the false cases in javadoc.
{code}
@@ -304,28 +308,40 @@ public abstract class ZKInterProcessLockBase implements
InterProcessLock {
}
/**
+ * Process metadata stored in a ZNode using a callback
+ * <p>
+ * @param lockZNode The node holding the metadata
+ * @return True if metadata was ready and processed
+ * @throws IOException If an unexpected ZooKeeper error occurs
+ * @throws InterruptedException If interrupted when reading the metadata
+ */
+ protected boolean handleLockMetadata(String lockZNode)
+ throws IOException {
+ return handleLockMetadata(lockZNode, handler);
+ }
+
+ /**
* Process metadata stored in a ZNode using a callback object passed to
* this instance.
* <p>
* @param lockZNode The node holding the metadata
+ * @param handler the metadata handler
* @return True if metadata was ready and processed
* @throws IOException If an unexpected ZooKeeper error occurs
* @throws InterruptedException If interrupted when reading the metadata
*/
{code}
help me here -- what does the magic value mean?
{code}
+ if (children.size() > 0) {
+ String currentLockHolder = getLockPath("lock-9999999999", children);
+ for (String child : children) {
+ String znode = ZKUtil.joinZNode(parentLockNode, child);
+
{code}
Ah -- here's the javadoc on the behavior of reapAll* and r/w aspects that I
asked about earlier I believe. Put this in the interface and then inheritdoc +
add the zk data here?
{code}
+ /**
+ * Will delete all lock znodes (read and write) which are "expired"
according to
+ * timeout. Assumption is that the clock skew between zookeeper and this
servers
+ * is negligible.
{code}
nice test in TestHBaseFsck.
[1]
http://hbase.apache.org/apidocs/org/apache/hadoop/hbase/zookeeper/lock/ZKInterProcessReadWriteLock.html#readLock(byte[])
> HBCK support for table locks
> ----------------------------
>
> Key: HBASE-8205
> URL: https://issues.apache.org/jira/browse/HBASE-8205
> Project: HBase
> Issue Type: Improvement
> Components: hbck, master, regionserver
> Reporter: Enis Soztutar
> Assignee: Enis Soztutar
> Fix For: 0.95.0, 0.98.0
>
> Attachments: hbase-8205_v1.patch, hbase-8205_v2.patch
>
>
> Table locks have been introduced in HBASE-7305, HBASE-7546, and others (see
> the design doc at HBASE-7305).
> This issue adds support in HBCK to report and fix possible conditions about
> table locks. Namely, if due to some bug, the table lock remains not-released,
> then HBCK should be able to report it, and remove the lock, so that normal
> table operations will continue.
> Also see the comments in HBASE-7977.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
