> On 2012-05-08 16:41:27, David Capwell wrote: > > storage-handlers/hbase/src/resources/revision-manager-default.xml, lines > > 22-35 > > <https://reviews.apache.org/r/5062/diff/1/?file=107668#file107668line22> > > > > Why only these two defaults and not all the other found in RM? > > > > public static final String DEFAULT_DATADIR = "/revision-management"; > > public static final String DEFAULT_HOSTLIST = "localhost:2181"; > > private static int DEFAULT_WRITE_TRANSACTION_TIMEOUT = 14400000; > > > > just to list a few > > Francis Liu wrote: > Hmmm, I'm actually hesitant about having defaults in more than one place. > Will have to rethink this.
You are doing that as part of this Jira. ZKBasedRevisionManager is defined as the default in three different files. There is no common class that hosts the keys and their defaults, so this information is scattered around. > 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. You are not doing that though, so why not just use the Constructor and if the need comes up then implement it. - 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 > >
