[GitHub] spark pull request #22989: [SPARK-25986][Build] Add rules to ban throw Error...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/22989#discussion_r233842829
--- Diff:
common/unsafe/src/main/java/org/apache/spark/unsafe/UnsafeAlignedOffset.java ---
@@ -39,7 +39,9 @@ public static int getSize(Object object, long offset) {
case 8:
return (int)Platform.getLong(object, offset);
default:
+// checkstyle.off: RegexpSinglelineJava
throw new AssertionError("Illegal UAO_SIZE");
--- End diff --
It would be OK too. This is just picking nits now but I thought
AssertionError was still better. ISE is about application state. Errors are
about JVM state.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22989: [SPARK-25986][Build] Add rules to ban throw Error...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/22989#discussion_r233821654
--- Diff:
common/unsafe/src/main/java/org/apache/spark/unsafe/UnsafeAlignedOffset.java ---
@@ -39,7 +39,9 @@ public static int getSize(Object object, long offset) {
case 8:
return (int)Platform.getLong(object, offset);
default:
+// checkstyle.off: RegexpSinglelineJava
throw new AssertionError("Illegal UAO_SIZE");
--- End diff --
yea, that's exactly the use case of `IllegalStateException`, which can also
pass the style check here.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22989: [SPARK-25986][Build] Add rules to ban throw Error...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/22989#discussion_r233713175
--- Diff:
common/unsafe/src/main/java/org/apache/spark/unsafe/UnsafeAlignedOffset.java ---
@@ -39,7 +39,9 @@ public static int getSize(Object object, long offset) {
case 8:
return (int)Platform.getLong(object, offset);
default:
+// checkstyle.off: RegexpSinglelineJava
throw new AssertionError("Illegal UAO_SIZE");
--- End diff --
I think these are ok as AssertionError because they shouldn't be able to
happen in any JVM state
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22989: [SPARK-25986][Build] Add rules to ban throw Error...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/22989#discussion_r233706605
--- Diff:
common/unsafe/src/main/java/org/apache/spark/unsafe/UnsafeAlignedOffset.java ---
@@ -52,7 +54,9 @@ public static void putSize(Object object, long offset,
int value) {
Platform.putLong(object, offset, value);
break;
default:
+// checkstyle.off: RegexpSinglelineJava
throw new AssertionError("Illegal UAO_SIZE");
--- End diff --
ditto
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22989: [SPARK-25986][Build] Add rules to ban throw Error...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/22989#discussion_r233706517
--- Diff:
common/unsafe/src/main/java/org/apache/spark/unsafe/UnsafeAlignedOffset.java ---
@@ -39,7 +39,9 @@ public static int getSize(Object object, long offset) {
case 8:
return (int)Platform.getLong(object, offset);
default:
+// checkstyle.off: RegexpSinglelineJava
throw new AssertionError("Illegal UAO_SIZE");
--- End diff --
shall we throw `IllegalStateException` here?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22989: [SPARK-25986][Build] Add rules to ban throw Error...
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/22989 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22989: [SPARK-25986][Build] Add rules to ban throw Error...
Github user xuanyuanking commented on a diff in the pull request:
https://github.com/apache/spark/pull/22989#discussion_r233432630
--- Diff:
mllib/src/test/scala/org/apache/spark/mllib/clustering/KMeansSuite.scala ---
@@ -331,7 +333,7 @@ class KMeansSuite extends SparkFunSuite with
MLlibTestSparkContext {
}
}
-object KMeansSuite extends SparkFunSuite {
+object KMeansSuite extends SparkFunSuite with Assertions {
--- End diff --
ah thanks! Done in 210d942.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22989: [SPARK-25986][Build] Add rules to ban throw Error...
Github user xuanyuanking commented on a diff in the pull request: https://github.com/apache/spark/pull/22989#discussion_r233432568 --- Diff: dev/checkstyle.xml --- @@ -180,5 +180,10 @@ + + + --- End diff -- `application code` against JVM here, as Error subclasses generally represent internal JVM errors. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22989: [SPARK-25986][Build] Add rules to ban throw Error...
Github user viirya commented on a diff in the pull request: https://github.com/apache/spark/pull/22989#discussion_r233318346 --- Diff: dev/checkstyle.xml --- @@ -180,5 +180,10 @@ + + + --- End diff -- What `application code` means here? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22989: [SPARK-25986][Build] Add rules to ban throw Error...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22989#discussion_r233317742
--- Diff:
mllib/src/test/scala/org/apache/spark/mllib/clustering/KMeansSuite.scala ---
@@ -331,7 +333,7 @@ class KMeansSuite extends SparkFunSuite with
MLlibTestSparkContext {
}
}
-object KMeansSuite extends SparkFunSuite {
+object KMeansSuite extends SparkFunSuite with Assertions {
--- End diff --
Isn't `Assertions` redundant?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22989: [SPARK-25986][Build] Add rules to ban throw Error...
Github user xuanyuanking commented on a diff in the pull request:
https://github.com/apache/spark/pull/22989#discussion_r232984147
--- Diff:
mllib/src/test/scala/org/apache/spark/ml/feature/VectorIndexerSuite.scala ---
@@ -283,7 +283,9 @@ class VectorIndexerSuite extends MLTest with
DefaultReadWriteTest with Logging {
points.zip(rows.map(_(0))).foreach {
case (orig: SparseVector, indexed: SparseVector) =>
assert(orig.indices.length == indexed.indices.length)
- case _ => throw new UnknownError("Unit test has a bug in it.")
// should never happen
+ case _ =>
+// should never happen
+throw new IllegalAccessException("Unit test has a bug in it.")
--- End diff --
Thanks, done in a4f49ce by just `fail` here.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22989: [SPARK-25986][Build] Add rules to ban throw Error...
Github user xuanyuanking commented on a diff in the pull request: https://github.com/apache/spark/pull/22989#discussion_r232983941 --- Diff: dev/checkstyle-suppressions.xml --- @@ -46,4 +46,12 @@ files="sql/catalyst/src/main/java/org/apache/spark/sql/streaming/GroupStateTimeout.java"/> +
[GitHub] spark pull request #22989: [SPARK-25986][Build] Add rules to ban throw Error...
Github user xuanyuanking commented on a diff in the pull request: https://github.com/apache/spark/pull/22989#discussion_r232955017 --- Diff: dev/checkstyle-suppressions.xml --- @@ -46,4 +46,12 @@ files="sql/catalyst/src/main/java/org/apache/spark/sql/streaming/GroupStateTimeout.java"/> +
[GitHub] spark pull request #22989: [SPARK-25986][Build] Add rules to ban throw Error...
Github user srowen commented on a diff in the pull request: https://github.com/apache/spark/pull/22989#discussion_r23291 --- Diff: dev/checkstyle-suppressions.xml --- @@ -46,4 +46,12 @@ files="sql/catalyst/src/main/java/org/apache/spark/sql/streaming/GroupStateTimeout.java"/> +
[GitHub] spark pull request #22989: [SPARK-25986][Build] Add rules to ban throw Error...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/22989#discussion_r232917995
--- Diff:
mllib/src/test/scala/org/apache/spark/ml/feature/VectorIndexerSuite.scala ---
@@ -283,7 +283,9 @@ class VectorIndexerSuite extends MLTest with
DefaultReadWriteTest with Logging {
points.zip(rows.map(_(0))).foreach {
case (orig: SparseVector, indexed: SparseVector) =>
assert(orig.indices.length == indexed.indices.length)
- case _ => throw new UnknownError("Unit test has a bug in it.")
// should never happen
+ case _ =>
+// should never happen
+throw new IllegalAccessException("Unit test has a bug in it.")
--- End diff --
Just `fail()` here? or at least not `IllegalAccessException`
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
