[
https://issues.apache.org/jira/browse/HBASE-5070?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13174648#comment-13174648
]
jirapos...@reviews.apache.org commented on HBASE-5070:
------------------------------------------------------
bq. On 2011-12-21 06:09:56, Michael Stack wrote:
bq. > This patch should go in. Good improvements. Doesn't address the bigger
issues of Configuration inline w/ HTD -- have you tried it, I mean, IIRC,
though you might have one custom config only, the whole Configuration will be
output per Constraint? -- and adding support to shell. Those are in different
issues?
bq.
bq. Jesse Yates wrote:
bq. I'm going to try checking out the conf stuff for the Constraints today
in the shell. Keep in mind that the conf stored per Constraint should just be
configuration info for the Constraint, as opposed to the entire hbase conf (so
we don't see all the values from hbase-site.xml, etc), but by default only see
the enabled and priority configuration values. I'm expecting that to be pretty
small, but I'll let you know.
bq.
bq. Also, there is HBASE-4879 to add shell support to Constraints as the
original was already pretty massive (and taking a while) and the shell stuff
could be cleanly separated out. I was working on it, but have temporarily
diverted from it to work on the maven modularization (4336) as per Ted's
request.
So looking at in the shell isn't all that bad, assuming people create their
configurations 'properly' - new Configuration(false) - and avoid loading the
default values into the configuration. If you forget to do that, describe is
going to be massive (10s of parameters scrolling by). However, I did a test
adding 3 random configuration values, and if you do it 'correctly', its just
looks like:
hbase(main):008:0> describe 'testTable'
DESCRIPTION
ENABLED
{NAME => 'testTable', constraint
$org.apache.hadoop.hbase.constraint.AllPassConstraint => '<?xml version="1.0"
encoding="UTF-8" s true
tandalone="no"?><configuration>
<property><name>anotherKey</name><value>anotherValue</value></property>
<property><name>_PRIORITY
</name><value>0</value></property>
<property><name>third key</name><value>thirdvalue</value></property>
<property><name>_ENABLED<
/name><value>true</value></property>
<property><name>someKey</name><value>someValue</value></property>
</configuration>', coproce
ssor$1 =>
'|org.apache.hadoop.hbase.constraint.ConstraintProcessor|1073741823|',
hbase.constraint.counter => '1', FAMILIES => []}
1 row(s) in 0.0340 seconds
Not that bad, IMO. There are some problems with ENABLED overlapping with the
actual values, but that should be sorted when I get around to fixing the shell
(not just 4879, but as we talked about on dev@ and then nothing happened.).
The only question that I have is then, how do we want to make people use
configurations? I'm thinking:
1) add Constraints.baseConfiguration(){ return new Configuration(false)}
2) Add information in the javadocs that you should create an empty configuration
3) Both
I'm pertinent to just (2), to save on what is really extraneous coding. Only
problem with that is, if you don't realize what you are doing (and there are a
lot of docs to read), then you could easily forget that boolean and create a
ton of massive configurations. But rtfm, right?
We could also leverage (1) to make sure that all the configurations we use have
a base set of properties (for now that would just be enabled and priority) and
then tell people to pull the configuration from that static method.
Thoughts?
- Jesse
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3273/#review4034
-----------------------------------------------------------
On 2011-12-20 19:14:46, Jesse Yates wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/3273/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-12-20 19:14:46)
bq.
bq.
bq. Review request for hbase, Gary Helmling, Ted Yu, and Michael Stack.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Follow-up on changes to constraint as per stack's comments on HBASE-4605.
bq.
bq.
bq. This addresses bug HBASE-5070.
bq. https://issues.apache.org/jira/browse/HBASE-5070
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. src/docbkx/book.xml bd3f881
bq. src/main/java/org/apache/hadoop/hbase/constraint/BaseConstraint.java
7ce6d45
bq. src/main/java/org/apache/hadoop/hbase/constraint/Constraint.java 2d8b4d7
bq. src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java
7825466
bq. src/main/java/org/apache/hadoop/hbase/constraint/package-info.java
6145ed5
bq.
src/test/java/org/apache/hadoop/hbase/constraint/CheckConfigurationConstraint.java
c49098d
bq.
bq. Diff: https://reviews.apache.org/r/3273/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. mvn clean test -P localTests -Dest=*Constraint* <- all tests pass.
bq.
bq.
bq. Thanks,
bq.
bq. Jesse
bq.
bq.
> Constraints implementation and javadoc changes
> ----------------------------------------------
>
> Key: HBASE-5070
> URL: https://issues.apache.org/jira/browse/HBASE-5070
> Project: HBase
> Issue Type: Task
> Reporter: Zhihong Yu
>
> This is continuation of HBASE-4605
> See Stack's comments https://reviews.apache.org/r/2579/#review3980
--
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
