[
https://issues.apache.org/jira/browse/HBASE-4605?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13172753#comment-13172753
]
[email protected] commented on HBASE-4605:
------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2579/#review3980
-----------------------------------------------------------
Very nice. Some comments below.
src/docbkx/book.xml
<https://reviews.apache.org/r/2579/#comment8986>
This needs filling out some; how do I add a constraint. Perhaps this is in
package info? If so, add link to it from here?
src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java
<https://reviews.apache.org/r/2579/#comment8987>
We're adding white space here
Should there be an answering String add method?
src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java
<https://reviews.apache.org/r/2579/#comment8988>
This just removes cp from HTD list. If loaded on a regionserver, it
continues to be so?
src/main/java/org/apache/hadoop/hbase/constraint/BaseConstraint.java
<https://reviews.apache.org/r/2579/#comment8989>
Constraints are in a constraint package? This sets up a precedent where
each cp gets its own package? Can't we have constraint in coprocessor package?
src/main/java/org/apache/hadoop/hbase/constraint/BaseConstraint.java
<https://reviews.apache.org/r/2579/#comment8990>
Should have used
http://hadoop.apache.org/common/docs/current/api/org/apache/hadoop/conf/Configurable.html?
src/main/java/org/apache/hadoop/hbase/constraint/Constraint.java
<https://reviews.apache.org/r/2579/#comment8994>
Nit: Missing 'of', 'not' should be 'no'
src/main/java/org/apache/hadoop/hbase/constraint/Constraint.java
<https://reviews.apache.org/r/2579/#comment8995>
'not' should be 'no'
src/main/java/org/apache/hadoop/hbase/constraint/Constraint.java
<https://reviews.apache.org/r/2579/#comment8996>
Missing 'should'?
src/main/java/org/apache/hadoop/hbase/constraint/Constraint.java
<https://reviews.apache.org/r/2579/#comment8997>
Nice class doc
src/main/java/org/apache/hadoop/hbase/constraint/Constraint.java
<https://reviews.apache.org/r/2579/#comment8998>
I can't have a constraint on a Delete? Should this be a Mutation?
src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java
<https://reviews.apache.org/r/2579/#comment9003>
key for sure is not going to be null?
src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java
<https://reviews.apache.org/r/2579/#comment9002>
Style: Braces if on separate line. No need of braces if 'if' test and if
statement are on same line. In fact this could have been written:
return value == null? null: new Pair<String, String>(key, value);
src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java
<https://reviews.apache.org/r/2579/#comment9006>
This method is about adding constraints but the body of the method seems to
be about upping priority. Should explain what priority upping has to do with
constraint adding?
src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java
<https://reviews.apache.org/r/2579/#comment9007>
Ain't these really fat? Ain't this going to bloat HTD?
src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java
<https://reviews.apache.org/r/2579/#comment9008>
This method is about Strings, not byte arrays?
src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java
<https://reviews.apache.org/r/2579/#comment9010>
This is going to serialize a fat Configuration into an HTD? If I list an
HTD in shell, its going to have this Configuration pollution in it?
src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java
<https://reviews.apache.org/r/2579/#comment9012>
Incomplete sentence
src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java
<https://reviews.apache.org/r/2579/#comment9014>
This is expensive call. Its done infrequently?
src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java
<https://reviews.apache.org/r/2579/#comment9015>
Can I ask whether a constraint enabled or disabled?
src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java
<https://reviews.apache.org/r/2579/#comment9016>
Braces.
src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java
<https://reviews.apache.org/r/2579/#comment9017>
For sure not null?
src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java
<https://reviews.apache.org/r/2579/#comment9026>
Where do I get one of these? Which classloader to use?
src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java
<https://reviews.apache.org/r/2579/#comment9027>
Should this be a priority comparator rather than a constraintcomparator?
src/main/java/org/apache/hadoop/hbase/constraint/package-info.java
<https://reviews.apache.org/r/2579/#comment9028>
Should there be protection against constraints being unloaded? For an
administrator only?
src/main/java/org/apache/hadoop/hbase/constraint/package-info.java
<https://reviews.apache.org/r/2579/#comment9029>
This text could do w/ a read-through/edit (see things like the curly-brace
here)
src/main/java/org/apache/hadoop/hbase/constraint/package-info.java
<https://reviews.apache.org/r/2579/#comment9032>
Do I have to alter the table after doing this?
src/main/java/org/apache/hadoop/hbase/constraint/package-info.java
<https://reviews.apache.org/r/2579/#comment9033>
Need a rolling restart
src/main/java/org/apache/hadoop/hbase/constraint/package-info.java
<https://reviews.apache.org/r/2579/#comment9034>
Is above missing the reenable of table after HTD editing?
This doc is excellent. Needs a pointer from book to here.
- Michael
On 2011-12-13 21:38:03, Jesse Yates wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2579/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-12-13 21:38:03)
bq.
bq.
bq. Review request for hbase.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Most of the implementation for adding constraints as a coprocessor.
bq.
bq. Looking for general comments on style/structure, though nitpicks are ok
too.
bq.
bq. Currently missing implementation for disableConstraints() since that will
require adding removeCoprocessor() to HTD (also comments on if this is worth it
would be good).
bq.
bq.
bq. This addresses bug HBASE-4605.
bq. https://issues.apache.org/jira/browse/HBASE-4605
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. src/docbkx/book.xml 9617950
bq. src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java 84a0d1a
bq. src/main/java/org/apache/hadoop/hbase/constraint/BaseConstraint.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/constraint/Constraint.java
PRE-CREATION
bq.
src/main/java/org/apache/hadoop/hbase/constraint/ConstraintException.java
PRE-CREATION
bq.
src/main/java/org/apache/hadoop/hbase/constraint/ConstraintProcessor.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/constraint/package-info.java
PRE-CREATION
bq. src/test/java/org/apache/hadoop/hbase/TestHTableDescriptor.java
PRE-CREATION
bq. src/test/java/org/apache/hadoop/hbase/constraint/AllFailConstraint.java
PRE-CREATION
bq. src/test/java/org/apache/hadoop/hbase/constraint/AllPassConstraint.java
PRE-CREATION
bq.
src/test/java/org/apache/hadoop/hbase/constraint/CheckConfigurationConstraint.java
PRE-CREATION
bq.
src/test/java/org/apache/hadoop/hbase/constraint/RuntimeFailConstraint.java
PRE-CREATION
bq. src/test/java/org/apache/hadoop/hbase/constraint/TestConstraint.java
PRE-CREATION
bq. src/test/java/org/apache/hadoop/hbase/constraint/TestConstraints.java
PRE-CREATION
bq. src/test/java/org/apache/hadoop/hbase/constraint/WorksConstraint.java
PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/2579/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Adding IntegrationTestConstraint and unit tests for Constraints and
IntegerConstraint. All of those pass.
bq.
bq.
bq. Thanks,
bq.
bq. Jesse
bq.
bq.
> Constraints
> -----------
>
> Key: HBASE-4605
> URL: https://issues.apache.org/jira/browse/HBASE-4605
> Project: HBase
> Issue Type: Improvement
> Components: client, coprocessors
> Affects Versions: 0.94.0
> Reporter: Jesse Yates
> Assignee: Jesse Yates
> Attachments: 4605-v6.txt, 4605.v7, constraint_as_cp.txt,
> java_Constraint_v2.patch, java_HBASE-4605_v1.patch, java_HBASE-4605_v2.patch,
> java_HBASE-4605_v3.patch, java_HBASE-4605_v5.patch, java_HBASE-4605_v7.patch
>
>
> From Jesse's comment on dev:
> {quote}
> What I would like to propose is a simple interface that people can use to
> implement a 'constraint' (matching the classic database definition). This
> would help ease of adoption by helping HBase more easily check that box, help
> minimize code duplication across organizations, and lead to easier adoption.
> Essentially, people would implement a 'Constraint' interface for checking
> keys before they are put into a table. Puts that are valid get written to the
> table, but if not people can will throw an exception that gets propagated
> back to the client explaining why the put was invalid.
> Constraints would be set on a per-table basis and the user would be expected
> to ensure the jars containing the constraint are present on the machines
> serving that table.
> Yes, people could roll their own mechanism for doing this via coprocessors
> each time, but this would make it easier to do so, so you only have to
> implement a very minimal interface and not worry about the specifics.
> {quote}
--
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
