[
https://issues.apache.org/jira/browse/HBASE-4605?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13139100#comment-13139100
]
[email protected] 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.
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.
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.
bq. On 2011-10-29 02:11:25, Gary Helmling wrote:
bq. > src/main/java/org/apache/hadoop/hbase/constraint/Constraint.java, line 12
bq. > <https://reviews.apache.org/r/2579/diff/1/?file=53673#file53673line12>
bq. >
bq. > Is this true? The HTD values are stored in a HashMap, so ordering
wouldn't be preserved.
We can add order preserving, if that makes sense. I can see it for cases where
you want to check the cheap constraints first and then do the expensive only if
the other ones pass.
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 like:
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.
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.
bq. On 2011-10-29 02:11:25, Gary Helmling wrote:
bq. >
src/main/java/org/apache/hadoop/hbase/constraint/ConstraintProcessor.java, line
80
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
RegionCoprocessorHost.
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.
bq. On 2011-10-29 02:11:25, Gary Helmling wrote:
bq. > src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java, line
75
bq. > <https://reviews.apache.org/r/2579/diff/1/?file=53675#file53675line75>
bq. >
bq. > I think this and the other addConstraints() method could just be
named add().
+1
bq. On 2011-10-29 02:11:25, Gary Helmling wrote:
bq. > src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java, line
124
bq. > <https://reviews.apache.org/r/2579/diff/1/?file=53675#file53675line124>
bq. >
bq. > 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?
bq. >
bq. > Also, I would just name this enable().
+1 That was some leftover cruft from development.
bq. On 2011-10-29 02:11:25, Gary Helmling wrote:
bq. > src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java, line
134
bq. > <https://reviews.apache.org/r/2579/diff/1/?file=53675#file53675line134>
bq. >
bq. > 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.
bq. >
bq. > 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.
Just trying to be explicit with the naming.
I like the idea of the remove, but we just need to make sure it gets well
documented as to what happens - I could see a people being confused.
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 values.
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
bq. On 2011-10-29 02:11:25, Gary Helmling wrote:
bq. > src/main/java/org/apache/hadoop/hbase/constraint/IntegerConstraint.java,
line 23
bq. > <https://reviews.apache.org/r/2579/diff/1/?file=53676#file53676line23>
bq. >
bq. > This doesn't seem right. Wouldn't most ints be stored as
Bytes.toBytes(int), not as a String?
Yeah, maybe. This is just an example. Depends on what people want to do...Maybe
the fact that the impl can vary so widely makes the case for this to be a test
class.
- Jesse
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2579/#review2917
-----------------------------------------------------------
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
