dongjoon-hyun commented on code in PR #58620:
URL: https://github.com/apache/spark/pull/58620#discussion_r4085349519
##########
core/src/main/scala/org/apache/spark/deploy/master/FileSystemPersistenceEngine.scala:
##########
@@ -84,6 +84,9 @@ private[master] class FileSystemPersistenceEngine(
}
}
+ // Unlike ZooKeeperPersistenceEngine, no recovery serialization filter is
applied here:
+ // the store is local to the master host; if it is corrupted, the master
cannot trust
Review Comment:
nit: "the store is local to the master host" isn't always true.
`docs/spark-standalone.md` explicitly describes mounting an NFS directory as
the recovery directory and restarting the Master on a different node. Maybe
word it as a lower-exposure surface rather than a local-only one.
##########
core/src/main/scala/org/apache/spark/deploy/master/ZooKeeperPersistenceEngine.scala:
##########
@@ -68,13 +75,48 @@ private[master] class ZooKeeperPersistenceEngine(conf:
SparkConf, val serializer
private def deserializeFromFile[T](filename: String)(implicit m:
ClassTag[T]): Option[T] = {
val fileData = zk.getData().forPath(workingDir + "/" + filename)
+ val recordingFilter = new RecordingFilter(serializationFilter)
try {
- Some(serializer.newInstance().deserialize[T](ByteBuffer.wrap(fileData)))
+ serializer.newInstance() match {
+ case javaInstance: JavaSerializerInstance =>
+ val in = javaInstance.deserializeStream(
+ new ByteBufferInputStream(ByteBuffer.wrap(fileData)),
recordingFilter)
+ try {
+ Some(in.readObject[T]())
+ } finally {
+ in.close()
+ }
+ case instance =>
+ Some(instance.deserialize[T](ByteBuffer.wrap(fileData)))
+ }
} catch {
+ case e: Exception if recordingFilter.rejected =>
+ // Rejected by the serialization filter, not found corrupt. Skip the
znode without
+ // deleting it: an overly narrow filter pattern (e.g.
"org.apache.spark.*", which
+ // does not match subpackages) must not wipe the whole recovery state
on failover.
+ logError(s"Skipping persisted file $filename, rejected by the recovery
" +
Review Comment:
nit: Could you use structured logging here, e.g. `log"Skipping persisted
file ${MDC(LogKeys.FILE_NAME, filename)}, rejected by the recovery
serialization filter (${MDC(LogKeys.CONFIG,
RECOVERY_SERIALIZATION_FILTER.key)})"`?
##########
docs/spark-standalone.md:
##########
@@ -857,6 +857,22 @@ In order to enable this recovery mode, you can set
SPARK_DAEMON_JAVA_OPTS in spa
</td>
<td>0.8.1</td>
</tr>
+ <tr>
+ <td><code>spark.deploy.recoverySerializationFilter</code></td>
Review Comment:
nit: Since skipped znodes are never cleaned up by the Master (and are logged
on every failover), could you mention that operators should inspect and remove
them manually? Also, since this is ZOOKEEPER-only, placing the row next to
`spark.deploy.zookeeper.dir` would read more naturally.
--
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]
