> On 2010-11-30 12:27:06, Jonathan Gray wrote: > > A little confused by the discrepancy between String host / int port and the > > Address. But does seem fine given we don't actually access the string/int > > values and always use the address object. > > > > Do we need some tests on this stuff? Seems like we always have issues here > > but tests don't catch anything. > > > > Looks better than what we have though so I'm +1 regardless.
Regarding tests, I'm not sure what they would catch... > On 2010-11-30 12:27:06, Jonathan Gray wrote: > > /trunk/src/main/java/org/apache/hadoop/hbase/HServerAddress.java, line 65 > > <http://review.cloudera.org/r/1262/diff/1/?file=17919#file17919line65> > > > > Why does stringValue not necessarily equal the host:port we store in > > those Strings? Shouldn't they be the same? I'm trying to keep it more consistent with the rest of the code, else when looking at the code you ask yourself the question you just asked me :) > On 2010-11-30 12:27:06, Jonathan Gray wrote: > > /trunk/src/main/java/org/apache/hadoop/hbase/HServerAddress.java, line 177 > > <http://review.cloudera.org/r/1262/diff/1/?file=17919#file17919line177> > > > > But on serialization, we use the address hostname not the thing we > > actually store in hostname/port variables, so after serialized it's > > different? > > > > Shouldn't we set the hostname/port variables on construction according > > to address.getAddress/getPort rather than the passed values, if the address > > values are what we want to use? I'm... not following you. You're saying that we shouldn't store the InetSocketAddress? > On 2010-11-30 12:27:06, Jonathan Gray wrote: > > /trunk/src/main/java/org/apache/hadoop/hbase/HServerInfo.java, line 116 > > <http://review.cloudera.org/r/1262/diff/1/?file=17920#file17920line116> > > > > I guess we never actually use the String host / int port? Why do we > > store them in HServerAddress then? Here I'm just making sure that after updating the address we also update the hostname, since it could have changed. - Jean-Daniel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1262/#review2012 ----------------------------------------------------------- On 2010-11-30 11:45:53, Jean-Daniel Cryans wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.cloudera.org/r/1262/ > ----------------------------------------------------------- > > (Updated 2010-11-30 11:45:53) > > > Review request for hbase. > > > Summary > ------- > > Changes: > - In HMaster, instead of passing an IP as String we now pass the HSA object > completely. > - In HRegionServer, I cleared a bunch of crufty comments and handle the HSA > passed by the master. > - In HServerInfo, I saw that the hostname wasn't reset when setting the HSA. > Fixed. > - In HServerAddress, I fixed a few places that wasn't explicitly using > hostnames and changed the serialization to pass a hostname instead of an IP > address. > > > This addresses bug HBASE-3286. > http://issues.apache.org/jira/browse/HBASE-3286 > > > Diffs > ----- > > /trunk/src/main/java/org/apache/hadoop/hbase/HServerAddress.java 1040669 > /trunk/src/main/java/org/apache/hadoop/hbase/HServerInfo.java 1040669 > /trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1040669 > > /trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java > 1040669 > > Diff: http://review.cloudera.org/r/1262/diff > > > Testing > ------- > > Works on my MBP (I was seeing the same issue but since there's only 1 RS it > didn't have any bad effect) and my 10 machines Ubuntu cluster. > > > Thanks, > > Jean-Daniel > >
