hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "jiraposter@reviews.apache.org (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-4605) Constraints
Date Sat, 29 Oct 2011 02:11:33 GMT

    [ https://issues.apache.org/jira/browse/HBASE-4605?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13139054#comment-13139054
] 

jiraposter@reviews.apache.org commented on HBASE-4605:
------------------------------------------------------


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2579/#review2917
-----------------------------------------------------------


Jessie, this looks like a pretty good start.  In addition to the individual comments, a couple
of general questions:

1) Should ConstraintProcessor support some standard way of mapping Constraint implementations
to the families/qualifiers they should apply to?
2) Should we ship a base set of Constraint implementations for common cases?

Even if the answer to both is "yes", they could be addressed as follow-up JIRAs.  But it would
be good to think through the end goal here.


src/main/java/org/apache/hadoop/hbase/constraint/Constraint.java
<https://reviews.apache.org/r/2579/#comment6534>

    I think this sentence needs updating to reference Constraints/ConstraintProcessor.



src/main/java/org/apache/hadoop/hbase/constraint/Constraint.java
<https://reviews.apache.org/r/2579/#comment6535>

    Is this true?  The HTD values are stored in a HashMap, so ordering wouldn't be preserved.



src/main/java/org/apache/hadoop/hbase/constraint/Constraint.java
<https://reviews.apache.org/r/2579/#comment6536>

    Should reference Constraint since BaseConstraint is just a convenience class.



src/main/java/org/apache/hadoop/hbase/constraint/Constraint.java
<https://reviews.apache.org/r/2579/#comment6521>

    Should we define a base exception here that is thrown?  I'm thinking something like:
    
    ConstraintViolationException extends DoNotRetryIOException
    
    This way we don't need to wrap in DoNotRetryIOException later and we give implementers
a bit more guidance on what to do.



src/main/java/org/apache/hadoop/hbase/constraint/ConstraintProcessor.java
<https://reviews.apache.org/r/2579/#comment6522>

    If we use a defined exception class for constraint violations (see comment above), I think
we can omit this try/catch block.  Expected exceptions would already subclass DoNotRetryIOException,
so no need to wrap it here.  Other (non-expected) Throwables would be handled higher up by
RegionCoprocessorHost.



src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java
<https://reviews.apache.org/r/2579/#comment6525>

    I think this and the other addConstraints() method could just be named add().



src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java
<https://reviews.apache.org/r/2579/#comment6523>

    I get the idea of being able to enable/disable constraints on demand, without removing
the individual constraints configured.  But why do we need this and the private addConstraintProcessor()
method?  Why not move the addConstraintProcessor() implementation here and make addConstraints()
call this instead?
    
    Also, I would just name this enable().



src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java
<https://reviews.apache.org/r/2579/#comment6524>

    In addition to being able to disable constraint checking by removing the ConstraintProcessor
CP, we probably also need a removeConstraints(HTableDescriptor) method that removes the ConstraintProcessor
CP and all the "constraint $" attributes on HTD for complete cleanup.
    
    By the way, since the name of the class is Constraints, I think this method could just
be named disable() -- the additional "Constraints" is redundant.



src/main/java/org/apache/hadoop/hbase/constraint/IntegerConstraint.java
<https://reviews.apache.org/r/2579/#comment6530>

    Is this intended as a test class or as a usable implementation?  If a test class, should
be under src/test/java/...
    
    If a usable implementation, then, looking at it, I wonder how useful it is without a way
of mapping constraints to individual qualifiers.  Do you think that's something ConstraintProcessor
should provide in general?
    
    It could be left up to end user implementations of Constraint to match which KeyValues
they should apply to.  But then I don't think any implementations we can bundle will have
much value, unless they provide for their own qualifier matching via configuration values.



src/main/java/org/apache/hadoop/hbase/constraint/IntegerConstraint.java
<https://reviews.apache.org/r/2579/#comment6532>

    This doesn't seem right.  Wouldn't most ints be stored as Bytes.toBytes(int), not as a
String?


- Gary


On 2011-10-26 22:34:35, 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-10-26 22:34:35)
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/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/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/IntegerConstraint.java PRE-CREATION

bq.    src/test/java/org/apache/hadoop/hbase/constraint/IntegrationTestConstraint.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: constraint_as_cp.txt, java_Constraint_v2.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

        

Mime
View raw message