[
https://issues.apache.org/jira/browse/HBASE-4605?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13172978#comment-13172978
]
[email protected] commented on HBASE-4605:
------------------------------------------------------
bq. On 2011-12-19 23:56:21, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java, line
413
bq. > <https://reviews.apache.org/r/2579/diff/8/?file=64473#file64473line413>
bq. >
bq. > This is expensive call. Its done infrequently?
Essentially, only when we update the configuration for a constraint - adding,
changing enabled or not, setting priority. The alternative was to keep around
the those values just as bytes in the value, but Gary's argument for keeping
them in the configuration for ease and compatibility going forward was the
basis for this change. These confs are expected to be very small, so this
shouldn't really be _that_ bad to write.
bq. On 2011-12-19 23:56:21, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/constraint/package-info.java, line
43
bq. > <https://reviews.apache.org/r/2579/diff/8/?file=64474#file64474line43>
bq. >
bq. > Should there be protection against constraints being unloaded? For
an administrator only?
Are there ways we can enforce that just admins can do certain things? Also, I
feel like if people are creating their own tables, they should be free to
modify their own tables. Clearly this gets into the security stuff of which
tables you should able to modify, which could be a bigger proposition. Thoughts?
bq. On 2011-12-19 23:56:21, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java, line
479
bq. > <https://reviews.apache.org/r/2579/diff/8/?file=64473#file64473line479>
bq. >
bq. > Can I ask whether a constraint enabled or disabled?
Yup - see enabled(HTD, Constraint).
bq. On 2011-12-19 23:56:21, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java, line
278
bq. > <https://reviews.apache.org/r/2579/diff/8/?file=64473#file64473line278>
bq. >
bq. > Ain't these really fat? Ain't this going to bloat HTD?
Yeah, it will bloat the HTD. Let's move this discussion down to comments on
Ted's review or onto 5070.
bq. On 2011-12-19 23:56:21, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java, line
166
bq. > <https://reviews.apache.org/r/2579/diff/8/?file=64473#file64473line166>
bq. >
bq. > key for sure is not going to be null?
it'll propagate a null pointer from the from the class->key conversion before
the return value here is null. What's the standard on handling the null pointer
issues client side? Ok to just let that propogate up? Defensive checks?
Wrapping it in some other exception? ??
bq. On 2011-12-19 23:56:21, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/constraint/BaseConstraint.java,
line 1
bq. > <https://reviews.apache.org/r/2579/diff/8/?file=64469#file64469line1>
bq. >
bq. > 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?
Having it in its own package was part of what I would consider making
constraints a "top-level" feature. It really depends on how we want to handle
doing constraints and CP related features going forward. For instance, the
security stuff is not only its own package, its going into its own module. I
would argue that constraints are less key than security, but important enough
to warrant its own subsection. Otherwise, we can get into a situation where we
either have a ton of classes in the coprocessors package (bad) or a ton of
packages - for each new feature (potentially way worse). I think we need to
treat them on a case-by-case basis going forward.
There are significant docs on constraints, making them not a great thing to
just drop into the coprocessor folder. Maybe that can be the metric - number of
lines of necessary documentation (as opposed to just all docs, to avoid people
creating filler just to have its own package).
bq. On 2011-12-19 23:56:21, Michael Stack wrote:
bq. > src/docbkx/book.xml, line 874
bq. > <https://reviews.apache.org/r/2579/diff/8/?file=64467#file64467line874>
bq. >
bq. > 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?
Should the link just be to the javadocs (which we could forget to update when
this release gets cut) or should it just be a comment that usage is further
explained in the javadocs/package-info?
bq. On 2011-12-19 23:56:21, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java, line 480
bq. > <https://reviews.apache.org/r/2579/diff/8/?file=64468#file64468line480>
bq. >
bq. > We're adding white space here
bq. >
bq. > Should there be an answering String add method?
We don't have an addValue for bytes, so I figured it was unnecessary.
bq. On 2011-12-19 23:56:21, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java, line
299
bq. > <https://reviews.apache.org/r/2579/diff/8/?file=64473#file64473line299>
bq. >
bq. > This method is about Strings, not byte arrays?
yup, this is from an earlier impl
bq. On 2011-12-19 23:56:21, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java, line
588
bq. > <https://reviews.apache.org/r/2579/diff/8/?file=64473#file64473line588>
bq. >
bq. > Should this be a priority comparator rather than a
constraintcomparator?
Internally, it is comparing the priorities, but those are local to the
constraints. What we really care about in the end is the order the constraints
are applied, hence, constraintComparator. But priority comparator could work
too.
bq. On 2011-12-19 23:56:21, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/constraint/package-info.java, line
65
bq. > <https://reviews.apache.org/r/2579/diff/8/?file=64474#file64474line65>
bq. >
bq. > This text could do w/ a read-through/edit (see things like the
curly-brace here)
hmmm, that line didn't make it into trunk, but giving it another once-over.
bq. On 2011-12-19 23:56:21, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/constraint/BaseConstraint.java,
line 26
bq. > <https://reviews.apache.org/r/2579/diff/8/?file=64469#file64469line26>
bq. >
bq. > Should have used
http://hadoop.apache.org/common/docs/current/api/org/apache/hadoop/conf/Configurable.html?
Even more so, probably Configured? Kinda makes this class a little obsolete...
I'm partial to keeping it in there, just to tie the two hierarchies together -
by implemnting BaseConstraint, all they need to take care of is overriding the
methods specified.
bq. On 2011-12-19 23:56:21, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java, line
173
bq. > <https://reviews.apache.org/r/2579/diff/8/?file=64473#file64473line173>
bq. >
bq. > 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?
Its in the package-info, but ok, here works too.
bq. On 2011-12-19 23:56:21, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java, line
317
bq. > <https://reviews.apache.org/r/2579/diff/8/?file=64473#file64473line317>
bq. >
bq. > 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?
Yeah, but there isn't a really good way of getting them across otherwise.
Follow-up should be in 5070 and to ted's comment below.
bq. On 2011-12-19 23:56:21, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java, line 1072
bq. > <https://reviews.apache.org/r/2579/diff/8/?file=64468#file64468line1072>
bq. >
bq. > This just removes cp from HTD list. If loaded on a regionserver, it
continues to be so?
I believe so - you should have to disable and then renable the table for it to
take effect (though it may be that you actually need to roll the RS).
bq. On 2011-12-19 23:56:21, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java, line
518
bq. > <https://reviews.apache.org/r/2579/diff/8/?file=64473#file64473line518>
bq. >
bq. > For sure not null?
yes...private method, so we should be able to expect valid elements - people
shouldn't be able to mess with the values here/put bad data.
- Jesse
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2579/#review3980
-----------------------------------------------------------
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
