You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "jschmidt10 (via GitHub)" <gi...@apache.org> on 2023/04/26 19:36:04 UTC
[GitHub] [accumulo] jschmidt10 opened a new issue, #3348: SplitUtils.getRangeLength can return a negative number
jschmidt10 opened a new issue, #3348:
URL: https://github.com/apache/accumulo/issues/3348
**Describe the bug**
The SplitUtils.getRangeLength() method can return a negative number (due to overflow) for certain Ranges. This became a problem for us using Spark 3.2.x where the default behavior is to ignore empty splits - they actually drop InputSplits where getLength() returns < 0.
**To Reproduce**
Here's a sample range:
```
Key begin = new Key("aaaaaaa");
Key end = new Key(new byte[] {
(byte) 0xFF,
(byte) 0xFF,
(byte) 0xFF,
(byte) 0xFF,
(byte) 0xFF,
(byte) 0xFF,
(byte) 0xFF
});
Range range = new Range(begin, end);
System.out.println(SplitUtils.getRangeLength(range));
```
That will print out an overflowed Long that has been flipped to a massively negative number.
**Expected behavior**
This should return a number > 0.
--
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.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on issue #3348: SplitUtils.getRangeLength can return a negative number
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on issue #3348:
URL: https://github.com/apache/accumulo/issues/3348#issuecomment-1541184199
> So do you think it's appropriate to just use Long.MAX_VALUE ...
My point was that the appropriate solution might be a case-by-case basis sort of thing. I'm not sure what value would be appropriate without digging in deep to figure out all the places where it's used.
> We could of course just handle a returned negative in both places but it still seems like the simplest thing is to have that method just return Long.MAX_VALUE as the computed range length if it detects an overflow/negative otherwise everytime we call the method we have to handle a potential negative returned which could lead to inconsistencies.
I think it was pointed out previously that the overflow could occur in some of the calling code anyway, so fixing in this method alone is not sufficient, and may be redundant to the fix in the calling code. My main point before was that the behavior of this method isn't important, because it's not public API, so long as the calling code handles it correctly, on a case-by-case basis.
--
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] cshannon commented on issue #3348: SplitUtils.getRangeLength can return a negative number
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on issue #3348:
URL: https://github.com/apache/accumulo/issues/3348#issuecomment-1537170960
> One thing to keep in mind, that may make this easier to address: SplitUtils is not public API and is not intended for direct consumption. It is used internally to help us approximate relative split sizes when calculating InputSplits. So, it doesn't matter if its method returns a negative number, as long as the places where it's used check its sign to ensure that they handle that situation appropriately.
>
> We could also try to come up with better approximation methods for split sizes, but I think addressing the negative in the places where it's used is the quickest and easiest way to fix this, without completely rewriting the approximation algorithm.
So do you think it's appropriate to just use Long.MAX_VALUE as the range length if a negative is detect in the spots where the method is used? (looks like [BatchInputSplit](https://github.com/apache/accumulo/blob/56d49f15a05db9a46dbceb845918497760601c11/hadoop-mapreduce/src/main/java/org/apache/accumulo/hadoopImpl/mapreduce/BatchInputSplit.java#L113) and RangeInputSplit currently)
We could of course just handle a returned negative in both places but it still seems like the simplest thing is to have that method just return Long.MAX_VALUE as the computed range length if it detects an overflow/negative otherwise everytime we call the method we have to handle a potential negative returned which could lead to inconsistencies.
--
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] ctubbsii commented on issue #3348: SplitUtils.getRangeLength can return a negative number
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on issue #3348:
URL: https://github.com/apache/accumulo/issues/3348#issuecomment-1530315463
One thing to keep in mind, that may make this easier to address: SplitUtils is not public API and is not intended for direct consumption. It is used internally to help us approximate relative split sizes when calculating InputSplits. So, it doesn't matter if its method returns a negative number, as long as the places where it's used check its sign to ensure that they handle that situation appropriately.
We could also try to come up with better approximation methods for split sizes, but I think addressing the negative in the places where it's used is the quickest and easiest way to fix this, without completely rewriting the approximation algorithm.
--
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] jschmidt10 commented on issue #3348: SplitUtils.getRangeLength can return a negative number
Posted by "jschmidt10 (via GitHub)" <gi...@apache.org>.
jschmidt10 commented on issue #3348:
URL: https://github.com/apache/accumulo/issues/3348#issuecomment-1530064131
@cshannon Thanks for checking it out. The MAX_VALUE sounds reasonable to me. Not sure how it's utilized in other non-Spark frameworks but seems like an estimate is reasonable and anything non-negative works.
--
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] cshannon commented on issue #3348: SplitUtils.getRangeLength can return a negative number
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on issue #3348:
URL: https://github.com/apache/accumulo/issues/3348#issuecomment-1528838346
I only briefly looked this over so it needs to be investigated in more detail but at first glance it seems like we could simply check for a negative value and return Long.MAX_VALUE instead of the negative when it overflows. We'd also need to be careful to not overflow elsewhere as well. For example, we iterate [here](https://github.com/apache/accumulo/blob/main/hadoop-mapreduce/src/main/java/org/apache/accumulo/hadoopImpl/mapreduce/BatchInputSplit.java#L113) and sum the total so that could overflow as well.
There is a [method](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/Math.html#addExact(long,long)) in the Math class that could be of use here to detect overflow but it's normally used for adding and this is doing bit shifting so not sure there are any utilities to help here.
--
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