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 Mon, 05 Dec 2011 01:53:41 GMT

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

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


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


Main concerns as same as previously -- the HTableDescriptor changes and IntegerConstraint:

1) HTD changes introduce a dependency on Guava.  I thought we had eliminated all client-side
dependencies on Guava?
2) I think serializing constraint configurations as XML would be useful, as they would then
be human readable and require less care to preserve previous serialization formats on upgrade
when the property implementations change.
3) I'm still not seeing the use of IntegerConstraint as the one Constraint implementation
that we ship as part of core (as opposed to it being an example for documentation).  Is there
a specific use where you're targeting this?


src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java
<https://reviews.apache.org/r/2579/#comment8047>

    I still don't think this gives us true encapsulation, since the HTD.setValue() allows
arbitrary byte[] for values, so there's no way of knowing if (DE)SERIALIZE_KEY was actually
used.  So is this really better than Bytes.toString()?
    
    I also thought that we had eliminated Guava as a client side dependency?  This would re-introduce
it.  That seems the bigger concern unless we've agreed to a client-side Guava dependency somewhere.
    
    Why not handle the values as Strings and serialize the Contraint configuration objects
as XML?  In this case, I think human readable is actually good.



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

    Omit copyright line



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

    Omit copyright line



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

    Should constraints apply to Increments and Deletes as well?  I don't know if they should
or not, and even if they do, it doesn't need to be part of this issue.  Just something to
consider.



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

    Omit copyright line



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

    Copyright line not necessary in file headers.



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

    Having HTD.getValue() convert to String here and then immediately converting back to byte[]
is odd.  I think either add HTD.getValueAsBytes() returning the raw byte[] or serialize the
Configuration as XML as I suggested and just handle it as a String here.



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

    The manual serialization of properties here seems likely to be brittle and subject to
breakage on upgrade if the implementation changes (unless backwards compatibility is explicitly
accounted for).
    
    This actually seems like a decent case for using the Configuration class' XML serialization
support.  The output will be much larger, but more resistant to changes in the properties
serialized.  Just clone the input configuration object, set the "enabled" and "priority" properties
under known keys, and serialize to a string using Configuration.writeXML().



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

    Is there a reason why this has to be included in code as a built in constraint implementation
(the only one), instead of just included in documentation as an example?
    
    As an example it makes sense to me.  As a the only built-in, it doesn't, since the way
it requires integers to be stored is at odds with all current usage, as I've previously point
out.


- Gary


On 2011-11-29 20:19:41, 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-11-29 20:19:41)
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 3c12169 
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/IntegerConstraint.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/IntegrationTestConstraint.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/TestConstraints.java PRE-CREATION

bq.    src/test/java/org/apache/hadoop/hbase/constraint/TestIntegerConstraint.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.v7, constraint_as_cp.txt, java_Constraint_v2.patch, java_HBASE-4605_v1.patch,
java_HBASE-4605_v2.patch, java_HBASE-4605_v3.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