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

Nicolas Liochon commented on HBASE-10497:
-----------------------------------------

I'm not sure of this one.
{code}
+++ 
hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSyncUp.java
  (working copy)
@@ -111,6 +111,7 @@
       }
     } catch (InterruptedException e) {
       System.err.println("didn't wait long enough:" + e);
+      Thread.currentThread().interrupt();
       return (-1);
     }
{code}
Because we took care of the interruption already (may be wrongly however), so 
the thread is not interrupted any more: the -1 means: we stop.
It's the same for the one with the split worker likely.

This one is likely wrong:
{code}
@@ -185,6 +185,8 @@
       try {
         Thread.sleep(100);
       } catch (InterruptedException ignored) {
+        LOG.warn("Interrupted while sleeping");
+        Thread.currentThread().interrupt();
       }
       if (System.currentTimeMillis() > startTime + 30000) {
         throw new RuntimeException("Master not active after 30 seconds");
{code}
As it's inside a loop, we're likely to loop again, then the sleep will be 
interrupted immediately as we restored the interruption status, then we will 
log again => we will flood the logs.

I haven't checked the whole patch, but inside a loop you can't simply restore 
the status, you need to take a decision (stop the loop) or store the 
interruption to restore the status later. When we can, it's better to take care 
of the interruption explicitly by stopping our process and/or rethrowing an 
exception to the caller. In the case above, may be be we should throw a runtime 
exception, as in "Master not active after 30 seconds"?

See as well ExceptionUtils (it's new).

> Add standard handling for swallowed InterruptedException thrown by 
> Thread.sleep under HBase-Client/HBase-Server folders systematically
> --------------------------------------------------------------------------------------------------------------------------------------
>
>                 Key: HBASE-10497
>                 URL: https://issues.apache.org/jira/browse/HBASE-10497
>             Project: HBase
>          Issue Type: Improvement
>          Components: Client, regionserver
>            Reporter: Feng Honghua
>            Assignee: Feng Honghua
>            Priority: Minor
>         Attachments: HBASE-10497-trunk_v1.patch
>
>
> There are many places where InterruptedException thrown by Thread.sleep are 
> swallowed silently (which are neither declared in the caller method's throws 
> clause nor rethrown immediately) under HBase-Client/HBase-Server folders.
> It'd be better to add standard 'log and call currentThread.interrupt' for 
> such cases.



--
This message was sent by Atlassian JIRA
(v6.1.5#6160)

Reply via email to