[
https://issues.apache.org/jira/browse/HCATALOG-404?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13270592#comment-13270592
]
[email protected] commented on HCATALOG-404:
--------------------------------------------------------
-----------------------------------------------------------
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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/5062/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-05-08 05:45:38)
bq.
bq.
bq. Review request for hcatalog, Vandana Ayyalasomayajula and Thomas.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. 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.
bq.
bq. I've added a new class and revision-*.xml artifacts.
bq.
bq.
bq. This addresses bug HCATALOG-404.
bq. https://issues.apache.org/jira/browse/HCATALOG-404
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. storage-handlers/hbase/build.xml 6653a66
bq. storage-handlers/hbase/conf/revision-manager-site.xml PRE-CREATION
bq.
storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/HBaseHCatStorageHandler.java
d3faa04
bq.
storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/snapshot/RevisionManagerConfiguration.java
PRE-CREATION
bq.
storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/snapshot/RevisionManagerEndpoint.java
55c97e0
bq.
storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/snapshot/RevisionManagerEndpointClient.java
3e03909
bq.
storage-handlers/hbase/src/java/org/apache/hcatalog/hbase/snapshot/RevisionManagerFactory.java
cffcf5e
bq. storage-handlers/hbase/src/resources/revision-manager-default.xml
PRE-CREATION
bq. storage-handlers/hbase/src/test/log4j.xml 8fd4fc9
bq.
storage-handlers/hbase/src/test/org/apache/hcatalog/hbase/snapshot/TestRevisionManagerConfiguration.java
PRE-CREATION
bq.
storage-handlers/hbase/src/test/org/apache/hcatalog/hbase/snapshot/TestRevisionManagerEndpoint.java
1e94fd2
bq.
bq. Diff: https://reviews.apache.org/r/5062/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Francis
bq.
bq.
> add separate configuration for revision manager
> -----------------------------------------------
>
> Key: HCATALOG-404
> URL: https://issues.apache.org/jira/browse/HCATALOG-404
> Project: HCatalog
> Issue Type: Improvement
> Affects Versions: 0.4.1
> Reporter: Francis Liu
> Assignee: Francis Liu
> Attachments: HCATALOG-404.patch
>
>
> revision manager currently expects some configs to be included in
> hbase-site.xml, which is complicates deployment and adding unrelated
> properties which are not native hbase.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
