Hiroshi Ikeda commented on HBASE-16642:

+  public synchronized long getTimeoutTimestamp() {
+    return getLastUpdate() + getTimeout();

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.

+  private static class DelayedContainer implements Delayed {
+    /** null if poison */
+    private final Procedure proc;
+    private final long timeoutTime;
+    public Procedure getProcedure() {
+      return proc;

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 

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

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...

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

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

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

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

Reply via email to