> On 2012-05-08 16:41:27, David Capwell wrote: > > storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/snapshot/RevisionManagerConfiguration.java, > > line 64 > > <https://reviews.apache.org/r/5062/diff/1/?file=107664#file107664line64> > > > > Why use merge when you can just use the Configuration constructor? The > > constructor should also be faster. > > Francis Liu wrote: > It's for cases where I'd like to modify an instance as opposed to > creating a new one. > > David Capwell wrote: > You are not doing that though, so why not just use the Constructor and if > the need comes up then implement it. > > Francis Liu wrote: > Actually I am doing that :-). See the create(Configuration) method. > > David Capwell wrote: > You are doing this in create(Config) > > Configuration conf = create(); // creates a new instance > merge(conf, that); > > Merge adds everything from the right side into the left. So in this case > the newly created configuration object. Since this is the only time merge is > used why not just use the constructor? > > Configuration conf = new Configuration(that); > addResources(conf); > return conf; > > Doing the above is faster than enumerating over the list + method calls > and is more clear what you are trying to do.
addResources() resets the values set in the configuration apart from the ones that are added as resources. - Francis ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5062/#review7686 ----------------------------------------------------------- On 2012-05-08 05:45:38, Francis Liu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5062/ > ----------------------------------------------------------- > > (Updated 2012-05-08 05:45:38) > > > Review request for hcatalog, Vandana Ayyalasomayajula and Thomas. > > > Summary > ------- > > revision manager currently expects some configs to be included in > hbase-site.xml, which complicates deployment as well as adding unrelated > properties which are not native hbase. > > I've added a new class and revision-*.xml artifacts. > > > This addresses bug HCATALOG-404. > https://issues.apache.org/jira/browse/HCATALOG-404 > > > Diffs > ----- > > storage-handlers/hbase/build.xml 6653a66 > storage-handlers/hbase/conf/revision-manager-site.xml PRE-CREATION > > storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/HBaseHCatStorageHandler.java > d3faa04 > > storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/snapshot/RevisionManagerConfiguration.java > PRE-CREATION > > storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/snapshot/RevisionManagerEndpoint.java > 55c97e0 > > storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/snapshot/RevisionManagerEndpointClient.java > 3e03909 > > storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/snapshot/RevisionManagerFactory.java > cffcf5e > storage-handlers/hbase/src/resources/revision-manager-default.xml > PRE-CREATION > storage-handlers/hbase/src/test/log4j.xml 8fd4fc9 > > storage-handlers/hbase/src/test/org/apache/hcatalog/hbase/snapshot/TestRevisionManagerConfiguration.java > PRE-CREATION > > storage-handlers/hbase/src/test/org/apache/hcatalog/hbase/snapshot/TestRevisionManagerEndpoint.java > 1e94fd2 > > Diff: https://reviews.apache.org/r/5062/diff > > > Testing > ------- > > > Thanks, > > Francis > >
