LuciferYang commented on a change in pull request #31517:
URL: https://github.com/apache/spark/pull/31517#discussion_r603754412
##########
File path:
core/src/main/scala/org/apache/spark/deploy/history/ApplicationCache.scala
##########
@@ -62,21 +64,27 @@ private[history] class ApplicationCache(
/**
* Removal event notifies the provider to detach the UI.
- * @param rm removal notification
+ * @param key removal key
+ * @param value removal value
*/
- override def onRemoval(rm: RemovalNotification[CacheKey, CacheEntry]):
Unit = {
+ override def onRemoval(key: CacheKey, value: CacheEntry,
+ cause: RemovalCause): Unit = {
metrics.evictionCount.inc()
- val key = rm.getKey
- logDebug(s"Evicting entry ${key}")
- operations.detachSparkUI(key.appId, key.attemptId,
rm.getValue().loadedUI.ui)
+ logDebug(s"Evicting entry $key")
+ operations.detachSparkUI(key.appId, key.attemptId, value.loadedUI.ui)
}
}
private val appCache: LoadingCache[CacheKey, CacheEntry] = {
- CacheBuilder.newBuilder()
- .maximumSize(retainedApplications)
- .removalListener(removalListener)
- .build(appLoader)
+ val builder = Caffeine.newBuilder()
+ .maximumSize(retainedApplications)
+ .removalListener(removalListener)
+ // SPARK-34309: Use custom Executor to compatible with
+ // the data eviction behavior of Guava cache
+ .executor((command: Runnable) => command.run())
+ // Wrapping as CaffeinatedGuava to be compatible with
+ // the exception behavior of Guava cache
+ CaffeinatedGuava.build(builder, appLoader)
Review comment:
I think we can remove the `CaffeinatedGuava` in the future, but it would
be better to keep it now for two reasons:
- For compatibility of exception handling behavior
For example, there are some differences in the processing of error, when the
`get` method throws an `Error`, Guava cache wraps it as an `ExecutionError`
and re-throw it, but Caffeine will re-throw `Error` directly. Different
exception types and nesting levels may lead to some incompatibilities, such as
`SPARK-33587: isFatalError` in `ExecutorSuite`, the test suite will fail if we
don't use `CaffeinatedGuava with Guava Cache Loader`.
- For compatibility of API usage
For example, `SessionCatalog` use `V get(K key, Callable<? extends V>
valueLoader)` defined in `com.google.common.cache.Cache`, but there is no same
interface in `caffeine.cache.Cache`.
In the future, maybe we can use use the `V get(K key, Function<? super K, ?
extends V> mappingFunction)` method to replace it
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
