dongjoon-hyun commented on code in PR #58467:
URL: https://github.com/apache/spark/pull/58467#discussion_r3914719995
##########
core/src/main/scala/org/apache/spark/status/protobuf/KVStoreProtobufSerializer.scala:
##########
@@ -51,6 +54,14 @@ private[spark] object KVStoreProtobufSerializer {
}.toMap
}
- def getSerializer(klass: Class[_]): Option[ProtobufSerDe[Any]] =
- serializerMap.get(klass)
+ private[this] val missedClasses = ConcurrentHashMap.newKeySet[Class[_]]()
+
+ def getSerializer(klass: Class[_]): Option[ProtobufSerDe[Any]] = {
+ val serializer = serializerMap.get(klass)
+ if (serializer.isEmpty && missedClasses.add(klass)) {
Review Comment:
This warning will fire on every normal startup, not only when a registration
is accidentally missing.
`KVUtils.open` always writes `AppStatusStoreMetadata` via
`db.setMetadata(...)` -> `RocksDB.put` -> `serializer.serialize`, and there is
(intentionally) no `ProtobufSerDe` for it. The live UI disk store always uses
`KVStoreProtobufSerializer` (`KVUtils.serializer(conf, live = true)`), and the
History Server does too with `spark.history.store.serializer=PROTOBUF`. The
same applies to the listing-store types (`FsHistoryProviderMetadata`,
`LogInfo`, history `ApplicationInfoWrapper`),
`HistoryServerDiskManager.ApplicationStoreInfo`, and the thriftserver
`SessionInfo` / `ExecutionInfo`.
So users will see e.g.
```
No Protobuf SerDe found for class
org.apache.spark.status.AppStatusStoreMetadata, falling back to use the JSON
SerDe.
```
on every start, which reads like a misconfiguration even though the JSON
fallback is by design for these classes. Shall we lower this to `logInfo` /
`logDebug`, or skip the known-intentional classes?
##########
core/src/test/scala/org/apache/spark/status/protobuf/KVStoreProtobufSerializerSuite.scala:
##########
@@ -35,6 +37,20 @@ import org.apache.spark.util.Utils.tryWithResource
class KVStoreProtobufSerializerSuite extends SparkFunSuite {
private val serializer = new KVStoreProtobufSerializer()
+ test("SPARK-59169: log a warning once per class when no ProtobufSerDe is
found") {
Review Comment:
nit: this assertion depends on the process-global `missedClasses` set never
having seen `FallbackTestData` before. It works today because this test runs
first and the class is unique to this suite, but it becomes order-dependent if
another test above it (or a re-run in the same JVM) serializes the class first.
Maybe worth a short comment, or a `private[protobuf]` reset hook for 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
