----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5062/#review7686 -----------------------------------------------------------
If you are finally going Configuration over Properties can we just pass around a configuration object and not a Properties object? Stuff like this really bothers me still https://github.com/apache/hcatalog/blob/trunk/storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/snapshot/RevisionManagerFactory.java#L82 storage-handlers/hbase/build.xml <https://reviews.apache.org/r/5062/#comment16926> I avoid ant as much as possible so this might be my ignorance but shouldn't that just be **/config/*? Also what files are in the jar? By convention the -default.xml should be in the jar but -site.xml can not. Is this true for the build? storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/snapshot/RevisionManagerConfiguration.java <https://reviews.apache.org/r/5062/#comment16924> Why use merge when you can just use the Configuration constructor? The constructor should also be faster. storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/snapshot/RevisionManagerEndpoint.java <https://reviews.apache.org/r/5062/#comment16927> Logger is from slf4j, can you use {} notation? storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/snapshot/RevisionManagerEndpointClient.java <https://reviews.apache.org/r/5062/#comment16928> Why switch from meta to root, is this just so it only runs on one server? storage-handlers/hbase/src/resources/revision-manager-default.xml <https://reviews.apache.org/r/5062/#comment16925> 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 storage-handlers/hbase/src/test/org/apache/hcatalog/hbase/snapshot/TestRevisionManagerConfiguration.java <https://reviews.apache.org/r/5062/#comment16929> Can you add a test that will use a RM stub, put that class in the config, then pass that config to the RM factory and check the class? Should verify that the conversion from config -> properties is working properly. - David 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 > >
