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 2020/06/19 17:09:53 UTC
[GitHub] [accumulo] cradal opened a new pull request #1637: #1618 Investigate TODO in exception message in new Bulk Import
cradal opened a new pull request #1637:
URL: https://github.com/apache/accumulo/pull/1637
----------------------------------------------------------------
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 edited a comment on pull request #1637: #1618 Investigate TODO in exception message in new Bulk Import
Posted by GitBox <gi...@apache.org>.
keith-turner edited a comment on pull request #1637:
URL: https://github.com/apache/accumulo/pull/1637#issuecomment-655705437
> @keith-turner You may go ahead and merge. Since the code did not change, nothing new was pushed.
@cradal If you let me know what name and/or email you would like to use I can change it when I squash.
----------------------------------------------------------------
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] cradal commented on a change in pull request #1637: #1618 Investigate TODO in exception message in new Bulk Import
Posted by GitBox <gi...@apache.org>.
cradal commented on a change in pull request #1637:
URL: https://github.com/apache/accumulo/pull/1637#discussion_r450962589
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/bulk/BulkImport.java
##########
@@ -616,7 +616,9 @@ private Text toText(byte[] row) {
if (containsPrevRow && containsEndRow) {
mappings.get(ke).merge(mappings.remove(oke));
} else {
- throw new RuntimeException("TODO handle merges");
+ throw new RuntimeException("Unable to execute merge. Key extent "
+ + oke.toString() + " extends beyond the range of Key extent "
+ + ke.toString() + ". Out of range data would be lost.");
Review comment:
@keith-turner Looks like contains() needs a ByteSequence or a BinaryComparable as the parameter. I will look into trying to convert the KeyExtents to one of these datatypes.
----------------------------------------------------------------
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 #1637: #1618 Investigate TODO in exception message in new Bulk Import
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1637:
URL: https://github.com/apache/accumulo/pull/1637#discussion_r442983640
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/bulk/BulkImport.java
##########
@@ -616,7 +616,9 @@ private Text toText(byte[] row) {
if (containsPrevRow && containsEndRow) {
mappings.get(ke).merge(mappings.remove(oke));
} else {
- throw new RuntimeException("TODO handle merges");
+ throw new RuntimeException("Unable to execute merge. Key extent "
+ + oke.toString() + " extends beyond the range of Key extent "
+ + ke.toString() + ". Out of range data would be lost.");
Review comment:
This wording may be confusing to users. Even though we are merging keys, it is happening mostly behind the scenes during a bulk import. Perhaps something like:
```suggestion
throw new RuntimeException("Error during bulk import: Unable to merge overlapping keys during a table split. Key extent " + oke.toString() + " extends beyond the range of " + ke.toString());
```
@keith-turner does this message make sense in the context of this method?
----------------------------------------------------------------
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] cradal commented on pull request #1637: #1618 Investigate TODO in exception message in new Bulk Import
Posted by GitBox <gi...@apache.org>.
cradal commented on pull request #1637:
URL: https://github.com/apache/accumulo/pull/1637#issuecomment-655593884
@keith-turner I updated my email locally. Should I re-push the same commit?
----------------------------------------------------------------
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 #1637: #1618 Investigate TODO in exception message in new Bulk Import
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1637:
URL: https://github.com/apache/accumulo/pull/1637#discussion_r451622230
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/bulk/BulkImport.java
##########
@@ -616,7 +616,9 @@ private Text toText(byte[] row) {
if (containsPrevRow && containsEndRow) {
mappings.get(ke).merge(mappings.remove(oke));
} else {
- throw new RuntimeException("TODO handle merges");
+ throw new RuntimeException("Unable to execute merge. Key extent "
+ + oke.toString() + " extends beyond the range of Key extent "
+ + ke.toString() + ". Out of range data would be lost.");
Review comment:
looks good @cradal
----------------------------------------------------------------
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 pull request #1637: #1618 Investigate TODO in exception message in new Bulk Import
Posted by GitBox <gi...@apache.org>.
keith-turner commented on pull request #1637:
URL: https://github.com/apache/accumulo/pull/1637#issuecomment-655583431
@cradal I was about to merge and noticed the email in your commit was `craig@pop-os.localdomain`. Was curious if you would like to change that before I merge. It can't be changed after merge.
----------------------------------------------------------------
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 pull request #1637: #1618 Investigate TODO in exception message in new Bulk Import
Posted by GitBox <gi...@apache.org>.
keith-turner commented on pull request #1637:
URL: https://github.com/apache/accumulo/pull/1637#issuecomment-655604795
> @keith-turner I updated my email locally. Should I re-push the same commit?
sure
----------------------------------------------------------------
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 pull request #1637: #1618 Investigate TODO in exception message in new Bulk Import
Posted by GitBox <gi...@apache.org>.
keith-turner commented on pull request #1637:
URL: https://github.com/apache/accumulo/pull/1637#issuecomment-655705437
> @keith-turner You may go ahead and merge. Since the code did not change, nothing new was pushed.
If you let me know what name and/or email you would like to use I can change it when I squash.
----------------------------------------------------------------
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 #1637: #1618 Investigate TODO in exception message in new Bulk Import
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1637:
URL: https://github.com/apache/accumulo/pull/1637#discussion_r451226815
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/bulk/BulkImport.java
##########
@@ -616,7 +616,9 @@ private Text toText(byte[] row) {
if (containsPrevRow && containsEndRow) {
mappings.get(ke).merge(mappings.remove(oke));
} else {
- throw new RuntimeException("TODO handle merges");
+ throw new RuntimeException("Unable to execute merge. Key extent "
+ + oke.toString() + " extends beyond the range of Key extent "
+ + ke.toString() + ". Out of range data would be lost.");
Review comment:
@cradal I was thinking the following code could be moved into a KeyExtent.contains(KeyExtent) method. Maybe something like the following.
```java
boolean contains(KeyExtent oke){
boolean containsPrevRow = getPrevEndRow() == null || (oke.getPrevEndRow() != null
&& getPrevEndRow().compareTo(oke.getPrevEndRow()) <= 0);
boolean containsEndRow = getEndRow() == null
|| (oke.getEndRow() != null && getEndRow().compareTo(oke.getEndRow()) >= 0);
return containsPrevRow && containsEndRow;
}
```
----------------------------------------------------------------
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 #1637: #1618 Investigate TODO in exception message in new Bulk Import
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1637:
URL: https://github.com/apache/accumulo/pull/1637#discussion_r443868607
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/bulk/BulkImport.java
##########
@@ -616,7 +616,9 @@ private Text toText(byte[] row) {
if (containsPrevRow && containsEndRow) {
mappings.get(ke).merge(mappings.remove(oke));
} else {
- throw new RuntimeException("TODO handle merges");
+ throw new RuntimeException("Unable to execute merge. Key extent "
+ + oke.toString() + " extends beyond the range of Key extent "
+ + ke.toString() + ". Out of range data would be lost.");
Review comment:
I looked into this a bit and I don't think the existing code is correct. I think it should do something like :
```java
if (ke.contains(oke)) {
mappings.get(ke).merge(mappings.remove(oke));
} else if (!oke.contains(ke)) {
throw new RuntimeException("Error during bulk import: Unable to merge overlapping tablets where neither tablet contains the other. This may be caused by a concurrent merge. Key extents " + oke + " and " + ke+" overlap, but neither contains the other"));
}
```
The above code assumes the code in this method to do the overlapping check is moved to KeyExtent.contains(KetExtent) to make the check simpler.
----------------------------------------------------------------
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] cradal commented on a change in pull request #1637: #1618 Investigate TODO in exception message in new Bulk Import
Posted by GitBox <gi...@apache.org>.
cradal commented on a change in pull request #1637:
URL: https://github.com/apache/accumulo/pull/1637#discussion_r451480967
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/bulk/BulkImport.java
##########
@@ -616,7 +616,9 @@ private Text toText(byte[] row) {
if (containsPrevRow && containsEndRow) {
mappings.get(ke).merge(mappings.remove(oke));
} else {
- throw new RuntimeException("TODO handle merges");
+ throw new RuntimeException("Unable to execute merge. Key extent "
+ + oke.toString() + " extends beyond the range of Key extent "
+ + ke.toString() + ". Out of range data would be lost.");
Review comment:
@keith-turner I added contains(KeyExtent oke) to the KeyExtent class and updated mergeOverlapping() in BulkImport accordingly.
----------------------------------------------------------------
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 pull request #1637: #1618 Investigate TODO in exception message in new Bulk Import
Posted by GitBox <gi...@apache.org>.
keith-turner commented on pull request #1637:
URL: https://github.com/apache/accumulo/pull/1637#issuecomment-656201740
@cradal I merged this in 25a55df2309fc1891ef208e35f2268b254ffb4e8. Please take a look at it to make sure I got it correct.
----------------------------------------------------------------
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] cradal commented on pull request #1637: #1618 Investigate TODO in exception message in new Bulk Import
Posted by GitBox <gi...@apache.org>.
cradal commented on pull request #1637:
URL: https://github.com/apache/accumulo/pull/1637#issuecomment-656056603
@keith-turner cdschei2@gmail.com Thanks!
----------------------------------------------------------------
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 closed pull request #1637: #1618 Investigate TODO in exception message in new Bulk Import
Posted by GitBox <gi...@apache.org>.
keith-turner closed pull request #1637:
URL: https://github.com/apache/accumulo/pull/1637
----------------------------------------------------------------
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] cradal commented on a change in pull request #1637: #1618 Investigate TODO in exception message in new Bulk Import
Posted by GitBox <gi...@apache.org>.
cradal commented on a change in pull request #1637:
URL: https://github.com/apache/accumulo/pull/1637#discussion_r450954434
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/bulk/BulkImport.java
##########
@@ -616,7 +616,9 @@ private Text toText(byte[] row) {
if (containsPrevRow && containsEndRow) {
mappings.get(ke).merge(mappings.remove(oke));
} else {
- throw new RuntimeException("TODO handle merges");
+ throw new RuntimeException("Unable to execute merge. Key extent "
+ + oke.toString() + " extends beyond the range of Key extent "
+ + ke.toString() + ". Out of range data would be lost.");
Review comment:
@keith-turner I agree this is better than the original. We can remove the 2 booleans from lines 611-614, making things more succinct. I will update the PR.
----------------------------------------------------------------
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 #1637: #1618 Investigate TODO in exception message in new Bulk Import
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1637:
URL: https://github.com/apache/accumulo/pull/1637#discussion_r451226815
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/bulk/BulkImport.java
##########
@@ -616,7 +616,9 @@ private Text toText(byte[] row) {
if (containsPrevRow && containsEndRow) {
mappings.get(ke).merge(mappings.remove(oke));
} else {
- throw new RuntimeException("TODO handle merges");
+ throw new RuntimeException("Unable to execute merge. Key extent "
+ + oke.toString() + " extends beyond the range of Key extent "
+ + ke.toString() + ". Out of range data would be lost.");
Review comment:
@cradal I was thinking this following code could be moved into a KeyExtent.contains(KeyExtent) method. Maybe something like the following.
```java
boolean contains(KeyExtent oke){
boolean containsPrevRow = getPrevEndRow() == null || (oke.getPrevEndRow() != null
&& getPrevEndRow().compareTo(oke.getPrevEndRow()) <= 0);
boolean containsEndRow = getEndRow() == null
|| (oke.getEndRow() != null && getEndRow().compareTo(oke.getEndRow()) >= 0);
return containsPrevRow && containsEndRow;
}
```
----------------------------------------------------------------
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 #1637: #1618 Investigate TODO in exception message in new Bulk Import
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1637:
URL: https://github.com/apache/accumulo/pull/1637#discussion_r443067013
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/bulk/BulkImport.java
##########
@@ -616,7 +616,9 @@ private Text toText(byte[] row) {
if (containsPrevRow && containsEndRow) {
mappings.get(ke).merge(mappings.remove(oke));
} else {
- throw new RuntimeException("TODO handle merges");
+ throw new RuntimeException("Unable to execute merge. Key extent "
+ + oke.toString() + " extends beyond the range of Key extent "
+ + ke.toString() + ". Out of range data would be lost.");
Review comment:
I will look at this next week.
----------------------------------------------------------------
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] cradal commented on pull request #1637: #1618 Investigate TODO in exception message in new Bulk Import
Posted by GitBox <gi...@apache.org>.
cradal commented on pull request #1637:
URL: https://github.com/apache/accumulo/pull/1637#issuecomment-656218117
@keith-turner Looks good.
----------------------------------------------------------------
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 pull request #1637: #1618 Investigate TODO in exception message in new Bulk Import
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1637:
URL: https://github.com/apache/accumulo/pull/1637#issuecomment-665399925
Did this PR replace #1636? I'm a bit confused as to what happened with that one.
----------------------------------------------------------------
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] cradal commented on pull request #1637: #1618 Investigate TODO in exception message in new Bulk Import
Posted by GitBox <gi...@apache.org>.
cradal commented on pull request #1637:
URL: https://github.com/apache/accumulo/pull/1637#issuecomment-655608893
@keith-turner You may go ahead and merge. Since the code did not change, nothing new was pushed.
----------------------------------------------------------------
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