----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7118/#review11609 -----------------------------------------------------------
looks good for me. several minor comments. 1) it would be better to modify bin/hedwig script to let it accept the region configuration file. 2) it might be better to provide a default config file for the region configuration like what we provided in bookkeeper-server/conf for bookie server. hedwig-server/src/main/java/org/apache/hedwig/server/netty/PubSubServer.java <https://reviews.apache.org/r/7118/#comment25026> a better name might be regionConfiguration to indicate it is a client configuration for crossing region. hedwig-server/src/main/java/org/apache/hedwig/server/regions/HedwigHubClientFactory.java <https://reviews.apache.org/r/7118/#comment25027> since a separated configuration file for crossing region client is introduced, I think we could deprecate the isInterRegionSSLEnabled. so we don't need to pass server configuration to HedwigHubClientFactory. for backward compatibility, you need to look into region configuration first to see whether ssl enabled. if ssl enabled, we don't need to look server conf; if ssl is disabled, we check server conf to see if InterRegionSSLEnabled is set or not. if InterRegionSSLEnabled is set, we set SSLEnabled to true in region configuration and pass it here. - Sijie Guo On Sept. 14, 2012, 6:28 p.m., Aniruddha Laud wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7118/ > ----------------------------------------------------------- > > (Updated Sept. 14, 2012, 6:28 p.m.) > > > Review request for bookkeeper, Ivan Kelly and Sijie Guo. > > > Description > ------- > > Make the region manager's client configurable. > > > Diffs > ----- > > > hedwig-server/src/main/java/org/apache/hedwig/server/netty/PubSubServer.java > 7220e23 > > hedwig-server/src/main/java/org/apache/hedwig/server/regions/HedwigHubClientFactory.java > fa240e2 > > hedwig-server/src/test/java/org/apache/hedwig/server/netty/TestPubSubServer.java > 5d64498 > > Diff: https://reviews.apache.org/r/7118/diff/ > > > Testing > ------- > > > Thanks, > > Aniruddha Laud > >
