[SPARK-42178][UI] Handle remaining null string values in ui protobuf serializer and add tests #39732
Conversation
|
cc @LuciferYang |
| @@ -31,6 +34,32 @@ import org.apache.spark.ui.scope.{RDDOperationEdge, RDDOperationNode} | |||
| class KVStoreProtobufSerializerSuite extends SparkFunSuite { | |||
| private val serializer = new KVStoreProtobufSerializer() | |||
|
|
|||
| test("All the string fields must be optional to avoid NPE") { | |||
There was a problem hiding this comment.
This test checks the protobuf file to make sure all the string fields are defined as optional string
There was a problem hiding this comment.
Example failure output:
ArrayBuffer((" string storage_level = 5;", 267), (" string name = 1;", 363)) was not empty
All the string fields should be defined as `optional string` for handling null string.
Please update the following fields:
line #267: string storage_level = 5;
line #363: string name = 1;
| val invalidDefinition = new mutable.ArrayBuffer[(String, Int)]() | ||
| var lineNumber = 1 | ||
| Source.fromFile(protoFile.toFile.getCanonicalPath).getLines().foreach { line => | ||
| if (line.matches(containsStringRegex)) { |
There was a problem hiding this comment.
org.apache.spark.util.Utils.tryWithResource should be used to ensureSource.fromFile(protoFile.toFile.getCanonicalPath) is closed after use
There was a problem hiding this comment.
+1 for the above comment.
| @@ -463,7 +492,7 @@ class KVStoreProtobufSerializerSuite extends SparkFunSuite { | |||
| name = null, | |||
| numPartitions = 8, | |||
| numCachedPartitions = 5, | |||
| storageLevel = "IN_MEMORY", | |||
| storageLevel = null, | |||
There was a problem hiding this comment.
Is this change required in this PR?
There was a problem hiding this comment.
yeah we need to test null string input. There is another object with storageLevel as IN_MEMORY already. Let's simply change the third object.
There was a problem hiding this comment.
+1, LGTM (except two comments; one from @LuciferYang and one question from me).
|
Another way I can think of is to check each |
|
@dongjoon-hyun @LuciferYang Thanks for the review. Merging to master/3.4 |
…serializer and add tests ### What changes were proposed in this pull request? * Similar to #39666, handle remaining null string values in ui protobuf serializer, including `RDDStorageInfo` and `ResourceInformation` * Add test to make sure all the string fields are defined as `optional string`. ### Why are the changes needed? Properly handles null string values in the protobuf serializer. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? New UTs Closes #39732 from gengliangwang/moreNullString. Authored-by: Gengliang Wang <gengliang@apache.org> Signed-off-by: Gengliang Wang <gengliang@apache.org>
What changes were proposed in this pull request?
RDDStorageInfoandResourceInformationoptional string.Why are the changes needed?
Properly handles null string values in the protobuf serializer.
Does this PR introduce any user-facing change?
No
How was this patch tested?
New UTs