You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/02/24 05:28:17 UTC

[GitHub] [spark] bozhang2820 opened a new pull request #35643: [WIP] Use error class in GraphiteSink

bozhang2820 opened a new pull request #35643:
URL: https://github.com/apache/spark/pull/35643


   ### What changes were proposed in this pull request?
   This change is to refactor exceptions thrown in GraphiteSink to use error class framework.
   
   ### Why are the changes needed?
   This is to follow the error class framework.
   
   ### Does this PR introduce _any_ user-facing change?
   No
   
   ### How was this patch tested?
   Added unit tests.


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] cloud-fan commented on a change in pull request #35643: [SPARK-38312][CORE] Use error class in GraphiteSink

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35643:
URL: https://github.com/apache/spark/pull/35643#discussion_r813674375



##########
File path: core/src/test/scala/org/apache/spark/metrics/sink/GraphiteSinkSuite.scala
##########
@@ -79,4 +79,42 @@ class GraphiteSinkSuite extends SparkFunSuite {
     assert(metricKeys.equals(filteredMetricKeys),
       "Should contain only metrics matches regex filter")
   }
+
+  test("GraphiteSink without host") {

Review comment:
       shall we have one big test suite for spark core errors? like `SparkCoreErrorSuite`? We have several of them in sql, such as `QueryCompilationErrorsSuite`




-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] bozhang2820 commented on a change in pull request #35643: [SPARK-38312][CORE] Use error class in GraphiteSink

Posted by GitBox <gi...@apache.org>.
bozhang2820 commented on a change in pull request #35643:
URL: https://github.com/apache/spark/pull/35643#discussion_r814376154



##########
File path: core/src/main/scala/org/apache/spark/metrics/sink/GraphiteSink.scala
##########
@@ -42,11 +43,13 @@ private[spark] class GraphiteSink(
   def propertyToOption(prop: String): Option[String] = Option(property.getProperty(prop))
 
   if (!propertyToOption(GRAPHITE_KEY_HOST).isDefined) {
-    throw new Exception("Graphite sink requires 'host' property.")
+    throw new SparkException(errorClass = "GRAPHITE_SINK_PROPERTY_MISSING",

Review comment:
       Sure. Will do.

##########
File path: core/src/main/scala/org/apache/spark/metrics/sink/GraphiteSink.scala
##########
@@ -42,11 +43,13 @@ private[spark] class GraphiteSink(
   def propertyToOption(prop: String): Option[String] = Option(property.getProperty(prop))
 
   if (!propertyToOption(GRAPHITE_KEY_HOST).isDefined) {
-    throw new Exception("Graphite sink requires 'host' property.")
+    throw new SparkException(errorClass = "GRAPHITE_SINK_PROPERTY_MISSING",

Review comment:
       Sure. Will do.

##########
File path: core/src/test/scala/org/apache/spark/metrics/sink/GraphiteSinkSuite.scala
##########
@@ -79,4 +79,42 @@ class GraphiteSinkSuite extends SparkFunSuite {
     assert(metricKeys.equals(filteredMetricKeys),
       "Should contain only metrics matches regex filter")
   }
+
+  test("GraphiteSink without host") {

Review comment:
       I think that's going to be hard since the ways to trigger spark-core errors in tests (so that we can catch and verify them) varies a lot.




-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] cloud-fan commented on a change in pull request #35643: [SPARK-38312][CORE] Use error class in GraphiteSink

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35643:
URL: https://github.com/apache/spark/pull/35643#discussion_r813674701



##########
File path: core/src/main/scala/org/apache/spark/metrics/sink/GraphiteSink.scala
##########
@@ -42,11 +43,13 @@ private[spark] class GraphiteSink(
   def propertyToOption(prop: String): Option[String] = Option(property.getProperty(prop))
 
   if (!propertyToOption(GRAPHITE_KEY_HOST).isDefined) {
-    throw new Exception("Graphite sink requires 'host' property.")
+    throw new SparkException(errorClass = "GRAPHITE_SINK_PROPERTY_MISSING",

Review comment:
       let's put the error in `SparkCoreErrors.scala`

##########
File path: core/src/main/scala/org/apache/spark/metrics/sink/GraphiteSink.scala
##########
@@ -42,11 +43,13 @@ private[spark] class GraphiteSink(
   def propertyToOption(prop: String): Option[String] = Option(property.getProperty(prop))
 
   if (!propertyToOption(GRAPHITE_KEY_HOST).isDefined) {
-    throw new Exception("Graphite sink requires 'host' property.")
+    throw new SparkException(errorClass = "GRAPHITE_SINK_PROPERTY_MISSING",

Review comment:
       let's put the error in `SparkCoreErrors.scala`




-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] bozhang2820 commented on pull request #35643: [WIP] Use error class in GraphiteSink

Posted by GitBox <gi...@apache.org>.
bozhang2820 commented on pull request #35643:
URL: https://github.com/apache/spark/pull/35643#issuecomment-1049575654


   @MaxGekk could you take a look at 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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] cloud-fan commented on pull request #35643: [SPARK-38312][CORE] Use error class in GraphiteSink

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35643:
URL: https://github.com/apache/spark/pull/35643#issuecomment-1058061782


   thanks, merging to master!


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] cloud-fan closed pull request #35643: [SPARK-38312][CORE] Use error class in GraphiteSink

Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #35643:
URL: https://github.com/apache/spark/pull/35643


   


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] dongjoon-hyun commented on pull request #35643: [SPARK-38312][CORE] Use error class in GraphiteSink

Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35643:
URL: https://github.com/apache/spark/pull/35643#issuecomment-1051187969


   I guess we need @cloud-fan 's final sign-off and merging.


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org