[
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)
