Github user squito commented on a diff in the pull request: https://github.com/apache/spark/pull/19583#discussion_r147522958 --- Diff: core/src/main/scala/org/apache/spark/HeartbeatReceiver.scala --- @@ -51,7 +51,26 @@ private case class ExecutorRegistered(executorId: String) private case class ExecutorRemoved(executorId: String) -private[spark] case class HeartbeatResponse(reregisterBlockManager: Boolean) +private[spark] case class UpdatedEpoch(epoch: Long) + +private[spark] object HeartbeatResponse { + def apply(reregisterBlockManager: Boolean, + updatedEpoch: Option[Long] = None): HeartbeatResponse = + updatedEpoch.fold[HeartbeatResponse](BasicHeartbeatResponse(reregisterBlockManager)) { + epoch => HeartbeatResponseWithEpoch(reregisterBlockManager, Some(epoch)) + } +} + +private[spark] sealed trait HeartbeatResponse { + def reregisterBlockManager: Boolean + def updatedEpoch: Option[Long] = None +} + +private[spark] case class BasicHeartbeatResponse(reregisterBlockManager: Boolean) + extends HeartbeatResponse +private[spark] case class HeartbeatResponseWithEpoch(reregisterBlockManager: Boolean, + override val updatedEpoch: Option[Long]) + extends HeartbeatResponse --- End diff -- the two levels of optional updatedEpoch -- (1) implementing class and (2) the `Option` itself seem unnecessary. I'd stick to just one type with an `Option`.
--- --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org