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 Tue, 29 Nov 2011 20:09:42 GMT

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

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



bq.  On 2011-11-29 08:36:29, Gary Helmling wrote:
bq.  > Most of what I see is just style issues, and a number of the HTableDescriptor changes
seem unnecessary.  If we clean up those, and assuming the latest patch has some documentation,
I think this is looking pretty good.

Uploading the latest (with some of your recommended changes) after this. Couple of concerns
may still need to be worked out around IntegerConstraint and the mods to HTD. Let me know
what you think.


bq.  On 2011-11-29 08:36:29, Gary Helmling wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/constraint/package-info.java, line 22
bq.  > <https://reviews.apache.org/r/2579/diff/6/?file=59947#file59947line22>
bq.  >
bq.  >     This differs from what seems to be the latest patch in JIRA (java_HBASE-4605_v3.txt).
 Are there other differences than the javadoc?
bq.  >     
bq.  >     For a patch of this size, review still belongs here in review board (even with
HadoopQA patch testing), so we should keep both versions in sync.  Please update the diff
here with the latest patch.

Yeah, this is the old version. Whats up on the ticket is just documentation. I'll put up the
latest here after making the changes.


bq.  On 2011-11-29 08:36:29, Gary Helmling wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java, line 250
bq.  > <https://reviews.apache.org/r/2579/diff/6/?file=59945#file59945line250>
bq.  >
bq.  >     if / else bodies should be contained in braces.

done.


bq.  On 2011-11-29 08:36:29, Gary Helmling wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java, line 233
bq.  > <https://reviews.apache.org/r/2579/diff/6/?file=59945#file59945line233>
bq.  >
bq.  >     Be consistent with braces here

Just following the requirements of java :) Done.


bq.  On 2011-11-29 08:36:29, Gary Helmling wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java, line 22
bq.  > <https://reviews.apache.org/r/2579/diff/6/?file=59945#file59945line22>
bq.  >
bq.  >     I see the usage outside of HTD now, but still don't see where this provides
more utility than Bytes.toString()

By pulling the functionality out of the HTD we can be sure that outside classes don't have
to worry about making sure they use the 'right' (de)serialization of they keys. Right now
we don't want to to anything more than just Bytes.toString(), but in the future we could make
it something more complex. For example, take the case where user specified keys are prepended
with a special identifier, so we can tell the difference between the two. Alternatively, we
could do some cool stuff to cut down on object creation (potentially) with the key conversion
or make it faster or more compact or w/e. 


Basically, I'm just adding the encapsulation so classes outside of the HTD don't have to know
about the fact that the HTD is using Bytes.toString() (and encapsulation is good, right?).
This way it is obvious, to an outside class, what is going - oh, it just serializing the key.
This makes the addCP code much cleaner too. 


bq.  On 2011-11-29 08:36:29, Gary Helmling wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/constraint/ConstraintProcessor.java, line
86
bq.  > <https://reviews.apache.org/r/2579/diff/6/?file=59944#file59944line86>
bq.  >
bq.  >     style nit: for loop body should be enclosed in braces

Done


bq.  On 2011-11-29 08:36:29, Gary Helmling wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/constraint/ConstraintProcessor.java, line
76
bq.  > <https://reviews.apache.org/r/2579/diff/6/?file=59944#file59944line76>
bq.  >
bq.  >     Add constraints.size() to this message and you don't need the separate LOG.debug()
message.

Done


bq.  On 2011-11-29 08:36:29, Gary Helmling wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/constraint/ConstraintProcessor.java, line
61
bq.  > <https://reviews.apache.org/r/2579/diff/6/?file=59944#file59944line61>
bq.  >
bq.  >     style nit: if body should be contained in braces

Done.


bq.  On 2011-11-29 08:36:29, Gary Helmling wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/constraint/IntegerConstraint.java, line 32
bq.  > <https://reviews.apache.org/r/2579/diff/6/?file=59946#file59946line32>
bq.  >
bq.  >     This still seems odd to me.  I can't see storing an integer value encoded as
a string, in order to be able to check for it being a valid integer.
bq.  >     
bq.  >     I would think most users would be storing integer values using Bytes.toInt(),
for which the checks here would not work.

The only problem with using the Bytes deserialization checking is that even if you put a string
in as bytes, it can be read out as an integer (in certain cases). Therefore, you could get
a case where something that should not be an integer is allowed when it really ought not to
be.


bq.  On 2011-11-29 08:36:29, Gary Helmling wrote:
bq.  > src/test/java/org/apache/hadoop/hbase/constraint/TestConstraints.java, line 90
bq.  > <https://reviews.apache.org/r/2579/diff/6/?file=59954#file59954line90>
bq.  >
bq.  >     brace alignment and indentation

Done.


bq.  On 2011-11-29 08:36:29, Gary Helmling wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java, line 1166
bq.  > <https://reviews.apache.org/r/2579/diff/6/?file=59940#file59940line1166>
bq.  >
bq.  >     I don't see why these changes are necessary when I don't see them being used
anywhere outside of HTD?  Do these additions buy us anything?  In my opinion, they're not
making the code any clearer than the current Byte.toString(), etc.

See response below.


- Jesse


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


On 2011-11-23 21:19:56, 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-23 21:19:56)
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/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