> 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.
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. - David ----------------------------------------------------------- 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 > >
