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

Patrick Angeles commented on HADOOP-7030:
-----------------------------------------

Hey Tom,

Thanks for the review. Here are some responses:

Could you combine the two types of file, so that if there are three columns the 
first two are interpreted as a range, otherwise use the first as a single host. 
Or just support CIDR notation?

- I'd prefer to keep them separate as the first two columns have completely 
different meanings when using one style (table lookup) over the other 
(IP-range).

Have you thought about InetAddress to avoid implementing IP address parsing 
logic? 
http://guava-libraries.googlecode.com/svn/tags/release08/javadoc/com/google/common/net/InetAddresses.html
 might be useful (there was talk of introducing Guava recently).

- I have not. Will look into this, although I'd rather keep this patch 
lightweight and not require the addition of another jar.

RefreshableDNSToSwitchMapping isn't hooked up yet, so perhaps it should go in a 
follow on JIRA.

- Yes, that is the intent. Those JIRAs would go into MAPREDUCE and HDFS.

The name "TableMapping" is a bit general. How about "FileBasedMapping", or 
similar?

- I'm willing to listen to suggestions, however I think FileBasedMapping is 
even more vague :)

The configuration keys should go in CommonConfigurationKeysPublic.

- Since this is a pluggable interface, I should not have to modify existing 
core code. That's better WRT separation of concerns and componentization. I'm 
willing to take your suggestion if the general consensus is that I should :)

Primes are not needed in hashCode implementations. For Ip4 
Arrays.hashCode(value) is sufficient.

- Ok.

The tests swallow exceptions - there should at least be a comment saying that 
this is expected. Also, fail() with a message is preferable to 
assertTrue(false).

- Ok.

The tests should be JUnit 4 style.

- Ok.

> new topology mapping implementations
> ------------------------------------
>
>                 Key: HADOOP-7030
>                 URL: https://issues.apache.org/jira/browse/HADOOP-7030
>             Project: Hadoop Common
>          Issue Type: New Feature
>    Affects Versions: 0.20.1, 0.20.2, 0.21.0
>            Reporter: Patrick Angeles
>            Assignee: Patrick Angeles
>         Attachments: HADOOP-7030-2.patch, HADOOP-7030.patch, topology.patch
>
>
> The default ScriptBasedMapping implementation of DNSToSwitchMapping for 
> determining cluster topology has some drawbacks. Principally, it forks to an 
> OS-specific script.
> This issue proposes two new Java implementations of DNSToSwitchMapping. 
> TableMapping reads a two column text file that maps an IP or hostname to a 
> rack ID. Ip4RangeMapping reads a three column text file where each line 
> represents a start and end IP range plus a rack ID.

-- 
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Reply via email to