[GitHub] [kafka] ijuma commented on a change in pull request #4090: [KAFKA-6084] Propagate JSON parsing errors in ReassignPartitionsCommand

[GitBox <gi...@apache.org>]

ijuma commented on a change in pull request #4090:
URL: https://github.com/apache/kafka/pull/4090#discussion_r430348986



##########
File path: core/src/main/scala/kafka/utils/Json.scala
##########
@@ -34,7 +34,7 @@ object Json {
    * Parse a JSON string into a JsonValue if possible. `None` is returned if `input` is not valid JSON.
    */
   def parseFull(input: String): Option[JsonValue] =
-    try Option(mapper.readTree(input)).map(JsonValue(_))
+    try doParseFull(input)
     catch { case _: JsonProcessingException => None }

Review comment:
       We can avoid the introduction of `doParseFull` by using `tryParseFull` in this method. Something like:
   `tryParseFull(input).toOption`. We can do the same for the other methods that return `Option`.

##########
File path: core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala
##########
@@ -1609,14 +1610,15 @@ object ReassignPartitionsCommand extends Logging {
   }
 
   def parsePartitionReassignmentData(jsonData: String): (Seq[(TopicPartition, Seq[Int])], Map[TopicPartitionReplica, String]) = {
-    Json.parseFull(jsonData) match {
-      case Some(js) =>
+    Json.tryParseFull(jsonData) match {
+      case Right(js) =>
         val version = js.asJsonObject.get("version") match {
           case Some(jsonValue) => jsonValue.to[Int]
           case None => EarliestVersion
         }
         parsePartitionReassignmentData(version, js)
-      case None => throw new AdminOperationException("The input string is not a valid JSON")
+      case Left(f) =>
+        throw f

Review comment:
       Will the calling code handle this exception? Or do we have to wrap it under `AdminOperationException`?

##########
File path: core/src/main/scala/kafka/utils/Json.scala
##########
@@ -65,6 +63,16 @@ object Json {
     catch { case e: JsonProcessingException => Left(e) }
   }
 
+  /**
+    * Parse a JSON string into a JsonValue if possible. It returns an `Either` where `Left` will be an exception and
+    * `Right` is the `JsonValue`.
+    * @param input a JSON string to parse
+    * @return An `Either` which in case of `Left` means an exception and `Right` is the actual return value.
+    */
+  def tryParseFull(input: String): Either[JsonProcessingException, JsonValue] =
+    try Right(mapper.readTree(input)).map(JsonValue(_))

Review comment:
       Checking Jackson's documentation, it says:
   
   > If no content is found from input (end-of-input), Java
   >      * <code>null</code> will be returned.
   
   So, we probably need to handle this ourselves and convert the returned `null` to an exception. The same issue affects other methods in this class that return `Either`.




----------------------------------------------------------------
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