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.
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).
The tests should be JUnit 4 style.