hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Hiroshi Ikeda (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-16642) Use DelayQueue instead of TimeoutBlockingQueue
Date Fri, 14 Oct 2016 02:25:20 GMT

    [ https://issues.apache.org/jira/browse/HBASE-16642?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15573895#comment-15573895
] 

Hiroshi Ikeda commented on HBASE-16642:
---------------------------------------

{code}
--- a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/Procedure.java
+++ b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/Procedure.java
...skip...
+  public synchronized long getTimeoutTimestamp() {
+    return getLastUpdate() + getTimeout();
{code}

The get/setLastUpdate is already guarded by this instance while the timeout escapes, and the
above synchronized is in vain.

The scattered synchronizations in Procedure would be buggy, although fixing it would require
to re-design the whole logic around that.

{code}
--- a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureExecutor.java
+++ b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureExecutor.java
...skip...
+  private static class DelayedContainer implements Delayed {
...skip...
+    /** null if poison */
+    private final Procedure proc;
+    private final long timeoutTime;
...skip...
+    public Procedure getProcedure() {
+      return proc;
{code}

The visibility restriction about the fields and the method is meaningless, since the outer
class can access any fields and methods in the nested class, while the classes outside the
outer class cannot access the private nested class.

{code}
+  private void addToWaitingQueue(final Procedure procedure) {
+    assert procedure.getState() == ProcedureState.WAITING_TIMEOUT;
+    waitingTimeout.add(new DelayedContainer(procedure));
+  }
{code}

Again, scattering synchronization in Procedure is ugly, and there must be some hidden additional
conditions around that logic otherwise the above assertion might fail. Since that is beyond
this issue let it alone for now...

{code}
   private void timeoutLoop() {
...skip...
+        task = waitingTimeout.take();
...skip...
+      if (task == null || task == DelayedContainer.POISON) {
{code}

DelayQueue doesn't permit null element (see javadoc) and take() never reutrns null.

{code}
-        if (proc.isRunnable()) {
+        if (proc.isWaiting()) {
{code}

Using lastUpdate instead of startTime, and Using isWaiting instead of isRunnable are beyond
this issue, and I can not say for sure about that.


> Use DelayQueue instead of TimeoutBlockingQueue
> ----------------------------------------------
>
>                 Key: HBASE-16642
>                 URL: https://issues.apache.org/jira/browse/HBASE-16642
>             Project: HBase
>          Issue Type: Sub-task
>          Components: proc-v2
>            Reporter: Hiroshi Ikeda
>            Assignee: Matteo Bertozzi
>            Priority: Minor
>             Fix For: 2.0.0
>
>         Attachments: HBASE-16642-v2.patch, HBASE-16642-v3.patch, HBASE-16642-v4.patch,
HBASE-16642.master.V1.patch
>
>
> Enqueue poisons in order to wake up and end the internal threads.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message