You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datasketches.apache.org by GitBox <gi...@apache.org> on 2022/01/11 23:28:19 UTC
[GitHub] [datasketches-java] leerho opened a new pull request #381: Fixes problems found when running latest version of SpotBugs.
leerho opened a new pull request #381:
URL: https://github.com/apache/datasketches-java/pull/381
The latest version of SpotBugs is much more aggressive especially in finding potential security issues. Some of these were false-positives, and a number only involved test code, which I'm not concerned about.
However, there was one that did concern me of SpotBugs type "EI_EXPOSE_REP2", "This code stores a reference to an externally mutable object into the internal representation of the object."
We do this in a number of places so I was surprised that SpotBugs did not find more. Nonetheless, the fix for this involved providing copy constructors in one hierarchy of tuple sketches. Although I fixed the bug that SpotBugs found, I'm sure there are more. This will require a more extensive review in the near future.
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] leerho merged pull request #381: Fixes problems found when running latest version of SpotBugs.
Posted by GitBox <gi...@apache.org>.
leerho merged pull request #381:
URL: https://github.com/apache/datasketches-java/pull/381
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] AlexanderSaydakov commented on a change in pull request #381: Fixes problems found when running latest version of SpotBugs.
Posted by GitBox <gi...@apache.org>.
AlexanderSaydakov commented on a change in pull request #381:
URL: https://github.com/apache/datasketches-java/pull/381#discussion_r782623228
##########
File path: src/main/java/org/apache/datasketches/sampling/VarOptItemsSketch.java
##########
@@ -785,6 +785,10 @@ T getItem(final int idx) {
return weights_.get(idx);
}
+ boolean isMarksValid() {
Review comment:
why expose this?
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] leerho commented on a change in pull request #381: Fixes problems found when running latest version of SpotBugs.
Posted by GitBox <gi...@apache.org>.
leerho commented on a change in pull request #381:
URL: https://github.com/apache/datasketches-java/pull/381#discussion_r782615213
##########
File path: src/main/java/org/apache/datasketches/quantiles/DirectUpdateDoublesSketch.java
##########
@@ -250,6 +251,11 @@ private WritableMemory growCombinedMemBuffer(final int itemSpaceNeeded) {
assert needBytes > memBytes;
memReqSvr = (memReqSvr == null) ? mem_.getMemoryRequestServer() : memReqSvr;
+ if (memReqSvr == null) {
+ throw new SketchesArgumentException(
+ "A reequest for more memory has been denied, "
Review comment:
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.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] leerho commented on a change in pull request #381: Fixes problems found when running latest version of SpotBugs.
Posted by GitBox <gi...@apache.org>.
leerho commented on a change in pull request #381:
URL: https://github.com/apache/datasketches-java/pull/381#discussion_r782639068
##########
File path: src/main/java/org/apache/datasketches/sampling/VarOptItemsSketch.java
##########
@@ -785,6 +785,10 @@ T getItem(final int idx) {
return weights_.get(idx);
}
+ boolean isMarksValid() {
Review comment:
If you look at what we had before in VarOptItemsSketchTest Lines 522-528 :
```
try {
sketch.getMark(0);
fail();
} catch (final NullPointerException e) {
// expected
}
```
SpotBugs identified that we were catching a NullPointerException and then swallowing it. That is a no-no. Best practice is to check for null, but never actually "catch" a NullPointerException with a swallow. We had a few of those. Even though this was in test. It is still bad practice, so I fixed it.
I added isMarksValid() and changed the test to
`assertFalse(sketch.isMarksValid());`
This avoids having to catch a NullPointerException and then swallow it.
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] AlexanderSaydakov commented on a change in pull request #381: Fixes problems found when running latest version of SpotBugs.
Posted by GitBox <gi...@apache.org>.
AlexanderSaydakov commented on a change in pull request #381:
URL: https://github.com/apache/datasketches-java/pull/381#discussion_r782643978
##########
File path: src/main/java/org/apache/datasketches/sampling/VarOptItemsSketch.java
##########
@@ -785,6 +785,10 @@ T getItem(final int idx) {
return weights_.get(idx);
}
+ boolean isMarksValid() {
Review comment:
Why not declare an expected exception from a test instead of exposing implementation details? Do we really need to test this at all? In C++ this get_mark() method is private.
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] AlexanderSaydakov commented on a change in pull request #381: Fixes problems found when running latest version of SpotBugs.
Posted by GitBox <gi...@apache.org>.
AlexanderSaydakov commented on a change in pull request #381:
URL: https://github.com/apache/datasketches-java/pull/381#discussion_r782611648
##########
File path: src/main/java/org/apache/datasketches/quantiles/DirectUpdateDoublesSketch.java
##########
@@ -250,6 +251,11 @@ private WritableMemory growCombinedMemBuffer(final int itemSpaceNeeded) {
assert needBytes > memBytes;
memReqSvr = (memReqSvr == null) ? mem_.getMemoryRequestServer() : memReqSvr;
+ if (memReqSvr == null) {
+ throw new SketchesArgumentException(
+ "A reequest for more memory has been denied, "
Review comment:
typo
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org