[SPARK-59169][CORE] Log a warning when KVStoreProtobufSerializer falls back to JSON SerDe by pan3793 · Pull Request #58467 · apache/spark

pan3793 · 2026-09-02T08:42:33Z

What changes were proposed in this pull request?

KVStoreProtobufSerializer.getSerializer now logs a warning when no ProtobufSerDe is registered for a class and it silently falls back to the JSON SerDe. Each missed class is recorded in a concurrent set so the warning is logged at most once per class, avoiding log flood.

Why are the changes needed?

The fallback is currently silent, so a missing ProtobufSerDe registration (e.g. a new UI store class without a corresponding serializer) is easy to miss and quietly loses the performance benefit of Protobuf serialization. A one-time warning makes the gap visible without flooding the log.

Does this PR introduce any user-facing change?

No, only a new warning log message.

How was this patch tested?

New UT in KVStoreProtobufSerializerSuite (core) verifies the warning content and the once-per-class dedup via withLogAppender. Existing KVStoreProtobufSerializerSuite suites in core and sql/core also pass.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Fable 5

…s back to JSON SerDe Assisted-by: Claude Fable 5

pan3793 · 2026-09-02T08:43:51Z

cc @LuciferYang @ulysses-you

ulysses-you · 2026-09-02T11:34:59Z

+  def getSerializer(klass: Class[_]): Option[ProtobufSerDe[Any]] = {
+    val serializer = serializerMap.get(klass)
+    if (serializer.isEmpty && missedClasses.add(klass)) {
+      logWarning(log"No Protobuf SerDe found for class ${MDC(CLASS_NAME, klass.getName)}, " +


I think debug level is enough, so nothing noisy by default. We can enhance the display log level by modifying the configuration file.

it's intended to log it at the warning level: JSON is the default option, and users likely switch to PROTOBUF for performance purposes, falling back means a performance drop.

for noise concerns, it's a one-time warning that makes the gap visible without flooding the log.

uros-b · 2026-09-02T12:35:47Z

-    serializerMap.get(klass)
+  private[this] val missedClasses = ConcurrentHashMap.newKeySet[Class[_]]()
+
+  def getSerializer(klass: Class[_]): Option[ProtobufSerDe[Any]] = {


Please take a look at KVStoreProtobufSerializerSuite. No test covers the new warning behavior. The PR description asserts "Existing tests" suffice, but both KVStoreProtobufSerializerSuite suites (core and sql/core) exercise only known-registered classes and never reach the serializer.isEmpty branch. A test using withLogAppender should be added to assert (a) a warning IS logged on the first call with an unregistered class, and (b) is NOT logged again on a repeated call for the same class (the deduplication invariant). Without this, the "warn exactly once" semantics are entirely untested.

Added a UT in KVStoreProtobufSerializerSuite (core) using withLogAppender, covering both the warning content and the once-per-class dedup, in 7dd4abe.

Assisted-by: Claude Fable 5

dongjoon-hyun · 2026-09-02T13:41:51Z

+
+  def getSerializer(klass: Class[_]): Option[ProtobufSerDe[Any]] = {
+    val serializer = serializerMap.get(klass)
+    if (serializer.isEmpty && missedClasses.add(klass)) {


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?

we started to experiment spark.history.store.serializer=PROTOBUF recently and found that there are many fallbacks, even though the feature has been developed since 3.4, so we want to expose them through warning logs to push us (and the community) to fix those missing implementations incrementally.

for the current missed classes you pointed out, I think we should implement protobuf serde for them shortly, then the warning logs will go away. and for any future added beans without protobuf serde, a warning log will signal developers to implement it.

so, I lean towards keeping it at the WARNING level, if you feel strongly, I can lower it to INFO to keep it visible by default.

dongjoon-hyun · 2026-09-02T13:41:52Z

 class KVStoreProtobufSerializerSuite extends SparkFunSuite {
  private val serializer = new KVStoreProtobufSerializer()

+  test("SPARK-59169: log a warning once per class when no ProtobufSerDe is found") {


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.

Added a private[protobuf] reset hook and the test now clears the set up front, so it no longer depends on ordering. Fixed in cea4510.

Assisted-by: Claude Fable 5

LuciferYang · 2026-09-02T14:50:19Z

+
+  def getSerializer(klass: Class[_]): Option[ProtobufSerDe[Any]] = {
+    val serializer = serializerMap.get(klass)
+    if (serializer.isEmpty && missedClasses.add(klass)) {


This warning fires wider than "missing registration": internal store values go through the same serializer, not just app types. The metadata classes are JSON by design -- every driver with spark.ui.store.path set warns for them on start -- and the backend's TypeAliases warns both when a type is first registered (serialize, so a brand-new store hits it too) and when a store with written entities is reopened (deserialize). Healthy deployments log warnings the operator cannot act on.

I'd skip the KVStore bookkeeping values: a Set of by-design-JSON classes in the object (the two metadata classes plus RocksDB.TypeAliases/LevelDB.TypeAliases), no log on hit. Separately, LogInfo in the SHS listing store also has no serde; that one is a real gap, so either add a serde for it or accept the warning, to be decided in a separate issue.

Partially done in 2b73e60: skipped RocksDB.TypeAliases/LevelDB.TypeAliases - they are kvstore-library internals the ProtobufSerDe SPI does not cover, so the warning is never actionable there.

Kept the warning for AppStatusStoreMetadata/FsHistoryProviderMetadata: SPARK-41053 scoped the SPIP to live UI, but SPARK-41685 (3.4.0) extended protobuf to SHS, where stores are opened in batch - these classes benefit from a serde and the warning flags them as real gaps, same as LogInfo. Will cover them in a separate JIRA.

LuciferYang · 2026-09-02T14:50:33Z

+    val serializer = serializerMap.get(klass)
+    if (serializer.isEmpty && missedClasses.add(klass)) {
+      logWarning(log"No Protobuf SerDe found for class ${MDC(CLASS_NAME, klass.getName)}, " +
+        log"falling back to use the JSON SerDe.")


Minor wording nit: "falling back to use the JSON SerDe" reads oddly -- fall back to takes a noun, e.g. "no ProtobufSerDe registered for ${class}, falling back to the JSON SerDe".

Fixed in 9912008.

Assisted-by: Claude Fable 5

[SPARK-59169][CORE] Log a warning when KVStoreProtobufSerializer fall…

a12f79c

…s back to JSON SerDe Assisted-by: Claude Fable 5

ulysses-you reviewed Sep 2, 2026

View reviewed changes

nit: remove log prefix

6976b68

uros-b reviewed Sep 2, 2026

View reviewed changes

address comment, add UT

7dd4abe

Assisted-by: Claude Fable 5

dongjoon-hyun reviewed Sep 2, 2026

View reviewed changes

make UT robust against test ordering

cea4510

Assisted-by: Claude Fable 5

LuciferYang reviewed Sep 2, 2026

View reviewed changes

pan3793 added 2 commits September 2, 2026 23:02

skip by-design JSON classes, fix wording

9912008

Assisted-by: Claude Fable 5

keep warning for metadata classes, skip only kvstore internals

2b73e60

Assisted-by: Claude Fable 5

Uh oh!

Conversation

pan3793 commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

pan3793 commented Sep 2, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pan3793 commented Sep 2, 2026 •

edited

Loading