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, 31 Oct 2011 19:55:33 GMT

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

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

bq.  On 2011-10-29 02:11:25, Gary Helmling wrote:
bq.  > Jessie, this looks like a pretty good start.  In addition to the individual comments,
a couple of general questions:
bq.  > 
bq.  > 1) Should ConstraintProcessor support some standard way of mapping Constraint implementations
to the families/qualifiers they should apply to?
bq.  > 2) Should we ship a base set of Constraint implementations for common cases?
bq.  > 
bq.  > 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.
bq.  Jesse Yates wrote:
bq.      1) I was thinking an individual constriant could check any/all of the cf/cq being
used. We can do it via (2) where we have an abstract Constraint that makes it easy to check
a given cf/cq.
bq.      So, no (kinda) and yes. For (2), I think it will be community usage/findings as to
what people find useful - we can decide on a case-by-case basis if it is a valid contribution.

Okay, seems fine to me.  Just wanted to raise the questions.  

bq.  On 2011-10-29 02:11:25, Gary Helmling wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/constraint/ConstraintProcessor.java, line
bq.  > <https://reviews.apache.org/r/2579/diff/1/?file=53674#file53674line80>
bq.  >
bq.  >     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
bq.  Jesse Yates wrote:
bq.      I don't think that runtime exceptions on a constraint should be allowed to propagate
up the CPHost  - if the constraint fails then that put should fail, but not anything else.

Some runtime exceptions could indicate programming errors or bugs, in which case I think it's
best to handle them the same way we handle unexpected errors in coprocessors -- by either
unloading or aborting, depending on configuration.  We need to be careful for what we allow
from user code running in process on region servers.

bq.  On 2011-10-29 02:11:25, Gary Helmling wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/constraint/IntegerConstraint.java, line 13
bq.  > <https://reviews.apache.org/r/2579/diff/1/?file=53676#file53676line13>
bq.  >
bq.  >     Is this intended as a test class or as a usable implementation?  If a test class,
should be under src/test/java/...
bq.  >     
bq.  >     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?
bq.  >     
bq.  >     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
bq.  Jesse Yates wrote:
bq.      Its meant to be both an example and a test util. I can move it into test, but I can
see cases where people might want to just check to make sure they only store values. Yes,
it a really simple use case, but it also helps people to build more complex ones

Maybe it should just go in the documentation then?  Either in package level javadoc or a section
of the HBase book?  Maybe longer term we need a separate "examples" maven module for this
and other extensions points.  But the current implementation doesn't seem useful out of the
box, since it forces integers to be stored as strings...  in order to check that they're integers.
 Seems odd.

bq.  On 2011-10-29 02:11:25, Gary Helmling wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/constraint/Constraint.java, line 39
bq.  > <https://reviews.apache.org/r/2579/diff/1/?file=53673#file53673line39>
bq.  >
bq.  >     Should we define a base exception here that is thrown?  I'm thinking something
bq.  >     
bq.  >     ConstraintViolationException extends DoNotRetryIOException
bq.  >     
bq.  >     This way we don't need to wrap in DoNotRetryIOException later and we give implementers
a bit more guidance on what to do.
bq.  Jesse Yates wrote:
bq.      I wanted to let people throw whatever exception they wanted to so they can be specific
as to what happened with minimal overhead. For instance, the IntegerConstraint lets the constraint
just check to and then throw a NumberFormatException if it fails. However, the code for the
user (the actual constraint implementation) is super concise and easy.

I disagree here.  Treating all possible exceptions the same seems wrong.  An OutOfMemoryError
is not equivalent to a NumberFormatException.  We need some structure in place for what types
of errors are allowed/expected, instead of swallowing everything and assuming it's just a
constraint violation.  I don't think asking the Constraint implementations to think about
what errors they want to throw is much of a burden.

- Gary

This is an automatically generated e-mail. To reply, visit:

On 2011-10-31 00:48:49, Jesse Yates wrote:
bq.  -----------------------------------------------------------
bq.  This is an automatically generated e-mail. To reply, visit:
bq.  https://reviews.apache.org/r/2579/
bq.  -----------------------------------------------------------
bq.  (Updated 2011-10-31 00:48:49)
bq.  Review request for hbase.
bq.  Summary
bq.  -------
bq.  Most of the implementation for adding constraints as a coprocessor. 
bq.  Looking for general comments on style/structure, though nitpicks are ok too. 
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.  This addresses bug HBASE-4605.
bq.      https://issues.apache.org/jira/browse/HBASE-4605
bq.  Diffs
bq.  -----
bq.    src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java 99875b8 
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.  Diff: https://reviews.apache.org/r/2579/diff
bq.  Testing
bq.  -------
bq.  Adding IntegrationTestConstraint and unit tests for Constraints and IntegerConstraint.
All of those pass.
bq.  Thanks,
bq.  Jesse

> 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


View raw message