[ https://issues.apache.org/jira/browse/HBASE-4605?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13159153#comment-13159153 ]
jirapos...@reviews.apache.org commented on HBASE-4605: ------------------------------------------------------ ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2579/#review3551 ----------------------------------------------------------- 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. src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java <https://reviews.apache.org/r/2579/#comment7921> Nice addition to have src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java <https://reviews.apache.org/r/2579/#comment7913> 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. src/main/java/org/apache/hadoop/hbase/constraint/ConstraintProcessor.java <https://reviews.apache.org/r/2579/#comment7916> style nit: if body should be contained in braces src/main/java/org/apache/hadoop/hbase/constraint/ConstraintProcessor.java <https://reviews.apache.org/r/2579/#comment7914> Add constraints.size() to this message and you don't need the separate LOG.debug() message. src/main/java/org/apache/hadoop/hbase/constraint/ConstraintProcessor.java <https://reviews.apache.org/r/2579/#comment7912> style nit: for loop body should be enclosed in braces src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java <https://reviews.apache.org/r/2579/#comment7917> I see the usage outside of HTD now, but still don't see where this provides more utility than Bytes.toString() src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java <https://reviews.apache.org/r/2579/#comment7918> Be consistent with braces here src/main/java/org/apache/hadoop/hbase/constraint/Constraints.java <https://reviews.apache.org/r/2579/#comment7915> if / else bodies should be contained in braces. src/main/java/org/apache/hadoop/hbase/constraint/IntegerConstraint.java <https://reviews.apache.org/r/2579/#comment7922> 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. I would think most users would be storing integer values using Bytes.toInt(), for which the checks here would not work. src/main/java/org/apache/hadoop/hbase/constraint/package-info.java <https://reviews.apache.org/r/2579/#comment7919> This differs from what seems to be the latest patch in JIRA (java_HBASE-4605_v3.txt). Are there other differences than the javadoc? 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. src/test/java/org/apache/hadoop/hbase/constraint/TestConstraints.java <https://reviews.apache.org/r/2579/#comment7920> brace alignment and indentation - Gary 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