hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Hari Krishna Dara (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-13014) Java Tool For Region Moving
Date Thu, 26 Feb 2015 12:49:05 GMT

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

Hari Krishna Dara commented on HBASE-13014:
-------------------------------------------

Some comments on {{readExcludes()}} method:

* The {{close()}} calls should be in a finally block.
* The below line doesn't need an initializer:
{code}
+      String line = new String();
{code}
* Replace the below block:
{code}
+      FileInputStream fis = new FileInputStream(f);
+      BufferedReader br = new BufferedReader(new InputStreamReader(fis));
{code}
with the below and just call {{br.close()}} (it will internally {[close()}} the underlying
readers and streams).
{code}
+      BufferedReader br = new BufferedReader(new FileReader(f));
{code}
* Support comment lines (skip lines starting with "#").
* You should use {{equals()}} in the below code:
{code}
+        if (line != "") {
{code}
like this:
{code}
+        if (line.equals("")) {
{code}
Better yet, use the {{StringUtils.isEmpty()}} from commons.

You can simplify the whole logic of {{stripServers()}}, if you just preconstruct the {{host:port}}
combination, something like this:
{code}
  private String stripServer(ArrayList<String> regionServers, String hostname, int port)
      throws Exception {
    String server = hostname+ServerName.SERVERNAME_SEPARATOR+port;
    if (! regionServers.remove(server)) {
      throw new Exception("Server: " + server + " is not Online");
    }
    return regionServers;
  }
{code}

Couple of {{close()}} calls in {{isSuccessfulScan()}} method that are not in {{finally}} block.

Some comments on {{getServerNameForRegion()}}
* A few {{close()}} calls not in {{finally}} block
* Returning {{null}} would be clearer here:
{code}
+      return server;
{code}
* Do you really intend to wait on {{tracker.isLocationAvailable()}} for ever? Is there a reason
for sleeping outside in {{Thread.sleep()}} instead of letting the tracker do the wait? I recommend
just using {{tracker.waitMetaRegionLocation()}} with an upper bound on {{timeout}} and fail
the operation if a {{null}} is returned, and this method can be interrupted too, if that is
what you were looking for.
* The outer {{new String()}} is not needed here:
{code}
+              new String(Bytes.toString(servername).replaceFirst(":", ",") + ","
+                  + Bytes.toLong(startcode));
{code}

Misc. others
* You have a couple of {{System.out.println()}}, you might want to replace with {{LOG}} calls.
* The below message could get misleading, since it could catch any unchecked exceptions also.
Better to create an exception type, or may be even just use the return value of {{stripServer()}}
to signal the error condition
{code}
+      } catch (Exception e) {
+        LOG.info("hostname=" + hostname + " is not up yet, waiting", e);
+      }
{code}
* Consider calling {{ExecutorService.shutdown()}} after adding all the tasks. The way it is
right now, we have the pool leaking (applies to both {{load()}} and {{unload()}}).

> Java Tool For Region Moving 
> ----------------------------
>
>                 Key: HBASE-13014
>                 URL: https://issues.apache.org/jira/browse/HBASE-13014
>             Project: HBase
>          Issue Type: Improvement
>            Reporter: Abhishek Singh Chouhan
>            Assignee: Abhishek Singh Chouhan
>         Attachments: HBASE-13014.patch
>
>
> As per discussion on HBASE-12989 we should move the functionality of region_mover.rb
into a Java tool and use region_mover.rb only only as a wrapper around it.



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

Mime
View raw message