You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2021/04/19 19:08:25 UTC
[GitHub] [accumulo] milleruntime opened a new pull request #2025: Fix bug in DefaultKeySizeConstraint
milleruntime opened a new pull request #2025:
URL: https://github.com/apache/accumulo/pull/2025
* Stop DefaultKeySizeConstraint from allowing keys with a large rowId that
did not have any column update mutations
* Add a test to DefaultKeySizeConstraintTest for this condition
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime closed pull request #2025: Fix bug in DefaultKeySizeConstraint
Posted by GitBox <gi...@apache.org>.
milleruntime closed pull request #2025:
URL: https://github.com/apache/accumulo/pull/2025
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2025: Fix bug in DefaultKeySizeConstraint
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2025:
URL: https://github.com/apache/accumulo/pull/2025#discussion_r617562712
##########
File path: core/src/main/java/org/apache/accumulo/core/constraints/DefaultKeySizeConstraint.java
##########
@@ -53,16 +53,14 @@ public String getViolationDescription(short violationCode) {
return NO_VIOLATIONS;
List<Short> violations = new ArrayList<>();
-
+ int size = mutation.getRow().length;
for (ColumnUpdate cu : mutation.getUpdates()) {
- int size = mutation.getRow().length;
Review comment:
> a user could very easily create a key larger than the constraint if they have a large row ID
The row length was being factored into the size of each column update which would equate to the eventual key size.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2025: Fix bug in DefaultKeySizeConstraint
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2025:
URL: https://github.com/apache/accumulo/pull/2025#discussion_r617739869
##########
File path: core/src/main/java/org/apache/accumulo/core/constraints/DefaultKeySizeConstraint.java
##########
@@ -53,16 +53,14 @@ public String getViolationDescription(short violationCode) {
return NO_VIOLATIONS;
List<Short> violations = new ArrayList<>();
-
+ int size = mutation.getRow().length;
for (ColumnUpdate cu : mutation.getUpdates()) {
- int size = mutation.getRow().length;
Review comment:
The current code seems to impose a limit on any total key size (4 dimensions: row + cf + cq + cv; doesn't include timestamp or internal delete flag dimensions). Your change seems to impose a limit on the total size of this mutation, and seems to turn it into a MutationSizeConstraint.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2025: Fix bug in DefaultKeySizeConstraint
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2025:
URL: https://github.com/apache/accumulo/pull/2025#discussion_r617617301
##########
File path: core/src/main/java/org/apache/accumulo/core/constraints/DefaultKeySizeConstraint.java
##########
@@ -53,16 +53,14 @@ public String getViolationDescription(short violationCode) {
return NO_VIOLATIONS;
List<Short> violations = new ArrayList<>();
-
+ int size = mutation.getRow().length;
for (ColumnUpdate cu : mutation.getUpdates()) {
- int size = mutation.getRow().length;
Review comment:
The `BatchWriter` at least prevents this from every happening and will throw an exception:
https://github.com/apache/accumulo/blob/675e41aaed0f8ae250d97a10dad35a8aef4c3e82/core/src/main/java/org/apache/accumulo/core/clientImpl/TabletServerBatchWriter.java#L247
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2025: Fix bug in DefaultKeySizeConstraint
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2025:
URL: https://github.com/apache/accumulo/pull/2025#discussion_r616871274
##########
File path: core/src/main/java/org/apache/accumulo/core/constraints/DefaultKeySizeConstraint.java
##########
@@ -53,16 +53,14 @@ public String getViolationDescription(short violationCode) {
return NO_VIOLATIONS;
List<Short> violations = new ArrayList<>();
-
+ int size = mutation.getRow().length;
for (ColumnUpdate cu : mutation.getUpdates()) {
- int size = mutation.getRow().length;
Review comment:
I wasn't sure but to me the `row ID` is an integral part of the Key and a user could very easily create a key larger than the constraint if they have a large `row ID`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2025: Fix bug in DefaultKeySizeConstraint
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2025:
URL: https://github.com/apache/accumulo/pull/2025#issuecomment-824132080
Closing this since it looks like it there are checks to prevent this situation from occurring.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2025: Fix bug in DefaultKeySizeConstraint
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2025:
URL: https://github.com/apache/accumulo/pull/2025#discussion_r617624979
##########
File path: core/src/main/java/org/apache/accumulo/core/constraints/DefaultKeySizeConstraint.java
##########
@@ -53,16 +53,14 @@ public String getViolationDescription(short violationCode) {
return NO_VIOLATIONS;
List<Short> violations = new ArrayList<>();
-
+ int size = mutation.getRow().length;
for (ColumnUpdate cu : mutation.getUpdates()) {
- int size = mutation.getRow().length;
Review comment:
> Yes but if there aren't any Column updates the row length will be ignored.
Ok I see what you are thinking about. Hopefully that case does not matter. When a mutation has no column updates, the batchwriter [should reject it](https://github.com/apache/accumulo/blob/8dfe443af55f5d3aacb12fb38c5002dcb61cb772/core/src/main/java/org/apache/accumulo/core/clientImpl/TabletServerBatchWriter.java#L246). Also if a mutation w/ no column update were to make it through somehow, when its inserted into the in memory map its [converted to keys/values](https://github.com/apache/accumulo/blob/8dfe443af55f5d3aacb12fb38c5002dcb61cb772/server/tserver/src/main/java/org/apache/accumulo/tserver/InMemoryMap.java#L423-L428). If column updates are empty, then no key values will be created.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2025: Fix bug in DefaultKeySizeConstraint
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2025:
URL: https://github.com/apache/accumulo/pull/2025#discussion_r617573072
##########
File path: core/src/main/java/org/apache/accumulo/core/constraints/DefaultKeySizeConstraint.java
##########
@@ -53,16 +53,14 @@ public String getViolationDescription(short violationCode) {
return NO_VIOLATIONS;
List<Short> violations = new ArrayList<>();
-
+ int size = mutation.getRow().length;
for (ColumnUpdate cu : mutation.getUpdates()) {
- int size = mutation.getRow().length;
Review comment:
Yes but if there aren't any Column updates the row length will be ignored. Adding this chunk of code to the test will cause it to fail now:
<pre>
m = new Mutation(oversized);
assertEquals(Collections.singletonList(DefaultKeySizeConstraint.MAX__KEY_SIZE_EXCEEDED_VIOLATION),
constraint.check(null, m));
</pre>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2025: Fix bug in DefaultKeySizeConstraint
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2025:
URL: https://github.com/apache/accumulo/pull/2025#issuecomment-823533331
I guess it doesn't really matter if it is a bug or not, the question should be whether or not this should be changed. Changing this Constraint could break existing tables with big Row IDs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2025: Fix bug in DefaultKeySizeConstraint
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2025:
URL: https://github.com/apache/accumulo/pull/2025#discussion_r616807473
##########
File path: core/src/main/java/org/apache/accumulo/core/constraints/DefaultKeySizeConstraint.java
##########
@@ -53,16 +53,14 @@ public String getViolationDescription(short violationCode) {
return NO_VIOLATIONS;
List<Short> violations = new ArrayList<>();
-
+ int size = mutation.getRow().length;
for (ColumnUpdate cu : mutation.getUpdates()) {
- int size = mutation.getRow().length;
Review comment:
The limited documentation on the class says it limits the size of keys. A column update would correspond to a key, so the existing code may correct. One change would be that it could break out of the loop after adding the first violation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org