You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2023/05/26 13:41:23 UTC
[GitHub] [accumulo] keith-turner opened a new pull request, #3431: adds validation to user split and new tests
keith-turner opened a new pull request, #3431:
URL: https://github.com/apache/accumulo/pull/3431
(no comment)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3431: adds validation to user split and new tests
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3431:
URL: https://github.com/apache/accumulo/pull/3431#discussion_r1207237717
##########
server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java:
##########
@@ -740,6 +740,23 @@ public void executeFateOperation(TInfo tinfo, TCredentials c, long opid, FateOpe
.map(ByteBufferUtil::toText).collect(Collectors.toCollection(TreeSet::new));
KeyExtent extent = new KeyExtent(tableId, endRow, prevEndRow);
+
+ if (splits.stream()
+ .anyMatch(split -> !extent.contains(split) || split.equals(extent.endRow()))) {
+ throw new ThriftTableOperationException(tableId.canonical(), null, tableOp,
+ TableOperationExceptionType.OTHER,
+ "Split is outside bounds of tablet or equal to the tablets endrow");
+ }
Review Comment:
> if the split is very large it might not be helpful to include in the logs.
I think logging them would be useful
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #3431: adds validation to user split and new tests
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #3431:
URL: https://github.com/apache/accumulo/pull/3431#discussion_r1206979540
##########
server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java:
##########
@@ -740,6 +740,23 @@ public void executeFateOperation(TInfo tinfo, TCredentials c, long opid, FateOpe
.map(ByteBufferUtil::toText).collect(Collectors.toCollection(TreeSet::new));
KeyExtent extent = new KeyExtent(tableId, endRow, prevEndRow);
+
+ if (splits.stream()
+ .anyMatch(split -> !extent.contains(split) || split.equals(extent.endRow()))) {
+ throw new ThriftTableOperationException(tableId.canonical(), null, tableOp,
+ TableOperationExceptionType.OTHER,
+ "Split is outside bounds of tablet or equal to the tablets endrow");
+ }
Review Comment:
I did just realize that we may not want to do this if the split is very large it might not be helpful to include in the logs.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner merged pull request #3431: adds validation to user split and new tests
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #3431:
URL: https://github.com/apache/accumulo/pull/3431
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #3431: adds validation to user split and new tests
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #3431:
URL: https://github.com/apache/accumulo/pull/3431#discussion_r1206981545
##########
server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java:
##########
@@ -740,6 +740,23 @@ public void executeFateOperation(TInfo tinfo, TCredentials c, long opid, FateOpe
.map(ByteBufferUtil::toText).collect(Collectors.toCollection(TreeSet::new));
KeyExtent extent = new KeyExtent(tableId, endRow, prevEndRow);
+
+ if (splits.stream()
+ .anyMatch(split -> !extent.contains(split) || split.equals(extent.endRow()))) {
+ throw new ThriftTableOperationException(tableId.canonical(), null, tableOp,
+ TableOperationExceptionType.OTHER,
+ "Split is outside bounds of tablet or equal to the tablets endrow");
+ }
Review Comment:
Also, is this case (split out of bounds) covered by a test?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3431: adds validation to user split and new tests
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3431:
URL: https://github.com/apache/accumulo/pull/3431#discussion_r1207302643
##########
server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java:
##########
@@ -740,6 +740,23 @@ public void executeFateOperation(TInfo tinfo, TCredentials c, long opid, FateOpe
.map(ByteBufferUtil::toText).collect(Collectors.toCollection(TreeSet::new));
KeyExtent extent = new KeyExtent(tableId, endRow, prevEndRow);
+
+ if (splits.stream()
+ .anyMatch(split -> !extent.contains(split) || split.equals(extent.endRow()))) {
+ throw new ThriftTableOperationException(tableId.canonical(), null, tableOp,
+ TableOperationExceptionType.OTHER,
+ "Split is outside bounds of tablet or equal to the tablets endrow");
+ }
Review Comment:
I added logging of the bad splits in 809ee38. Used an internal function that truncates data to avoid putting too much data in the logs.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #3431: adds validation to user split and new tests
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #3431:
URL: https://github.com/apache/accumulo/pull/3431#discussion_r1206978775
##########
server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java:
##########
@@ -740,6 +740,23 @@ public void executeFateOperation(TInfo tinfo, TCredentials c, long opid, FateOpe
.map(ByteBufferUtil::toText).collect(Collectors.toCollection(TreeSet::new));
KeyExtent extent = new KeyExtent(tableId, endRow, prevEndRow);
+
+ if (splits.stream()
+ .anyMatch(split -> !extent.contains(split) || split.equals(extent.endRow()))) {
+ throw new ThriftTableOperationException(tableId.canonical(), null, tableOp,
+ TableOperationExceptionType.OTHER,
+ "Split is outside bounds of tablet or equal to the tablets endrow");
+ }
Review Comment:
Not sure if it would be helpful to include more info in these new exception messages but if so, we could do something like this:
```java
Optional<Text> badSplit = splits.stream()
.filter(split -> !extent.contains(split) || split.equals(extent.endRow())).findAny();
if (badSplit.isPresent()) {
Text split = badSplit.orElseThrow();
throw new ThriftTableOperationException(tableId.canonical(), null, tableOp,
TableOperationExceptionType.OTHER,
"Split '" + split + "' is outside bounds of tablet or equal to the tablets endrow '"
+ extent.endRow() + "'");
}
```
Similar approach could apply to the other exception as well.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3431: adds validation to user split and new tests
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3431:
URL: https://github.com/apache/accumulo/pull/3431#discussion_r1207240039
##########
server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java:
##########
@@ -740,6 +740,23 @@ public void executeFateOperation(TInfo tinfo, TCredentials c, long opid, FateOpe
.map(ByteBufferUtil::toText).collect(Collectors.toCollection(TreeSet::new));
KeyExtent extent = new KeyExtent(tableId, endRow, prevEndRow);
+
+ if (splits.stream()
+ .anyMatch(split -> !extent.contains(split) || split.equals(extent.endRow()))) {
+ throw new ThriftTableOperationException(tableId.canonical(), null, tableOp,
+ TableOperationExceptionType.OTHER,
+ "Split is outside bounds of tablet or equal to the tablets endrow");
+ }
Review Comment:
> Also, is this case (split out of bounds) covered by a test?
This one is really tricky to test because would only see it in practice if the client code has bug. If the client code is working properly it will only send rows that are within the extent. So can not test this via API calls, would need to directly send bad data to the thrift API which is not something I am sure I want to do an IT because it would be a lot of code.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org