-
Notifications
You must be signed in to change notification settings - Fork 188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace caffeine cache with reference to full map #2214
Changes from 7 commits
3b66cc1
72348b6
bc29c9c
652d42a
df17bbe
8cb4138
a020b92
7a77ce2
80bb901
1e95103
7b1ca90
e8af8b1
ee556cd
50e1dee
69ebebc
f7174d3
488b35c
dd24913
5947b22
6a0732b
9373f5e
1517b59
ebaa6ed
b2a1f25
e3e3ea2
f2a2e22
5b2069b
bfbcf36
642ebe1
c23fcaf
76aaa10
14a9ab9
9e7555b
efcd68b
5b97f8b
254d6e4
12d6162
862c22e
3514f4b
9cafb6c
2223fdb
9cdd205
73875d6
6f2849b
e7bc9d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -446,8 +446,9 @@ public class SingularityConfiguration extends Configuration { | |
// Enable caffeine cache on heavily requested endpoint | ||
private boolean useCaffeineCache = false; | ||
|
||
// Caffeine cache ttl | ||
private int caffeineCacheTtl = 1; | ||
// Caffeine cache TTLs | ||
private int deployCaffeineCacheTtl = 1; | ||
private int requestCaffeineCacheTtl = 1; | ||
|
||
public long getAskDriverToKillTasksAgainAfterMillis() { | ||
return askDriverToKillTasksAgainAfterMillis; | ||
|
@@ -2084,11 +2085,19 @@ public void setUseCaffeineCache(boolean useCaffeineCache) { | |
this.useCaffeineCache = useCaffeineCache; | ||
} | ||
|
||
public int getCaffeineCacheTtl() { | ||
return caffeineCacheTtl; | ||
public int getDeployCaffeineCacheTtl() { | ||
return deployCaffeineCacheTtl; | ||
} | ||
|
||
public void setCaffeineCacheTtl(int caffeineCacheTtl) { | ||
this.caffeineCacheTtl = caffeineCacheTtl; | ||
public void setDeployCaffeineCacheTtl(int deployCaffeineCacheTtl) { | ||
this.deployCaffeineCacheTtl = deployCaffeineCacheTtl; | ||
} | ||
|
||
public int getRequestCaffeineCacheTtl() { | ||
return requestCaffeineCacheTtl; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One last nit — can we append the time unit to this so users don't have to search code for it? |
||
} | ||
|
||
public void setRequestCaffeineCacheTtl(int requestCaffeineCacheTtl) { | ||
this.requestCaffeineCacheTtl = requestCaffeineCacheTtl; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
package com.hubspot.singularity.data; | ||
|
||
import com.github.benmanes.caffeine.cache.Caffeine; | ||
import com.github.benmanes.caffeine.cache.LoadingCache; | ||
import com.google.inject.Inject; | ||
import java.util.Map; | ||
import java.util.concurrent.TimeUnit; | ||
import java.util.function.Function; | ||
import javax.annotation.Nonnull; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
public class ManagerCache<K, V> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check out the ZkCache class. It's basically the same thing as this and might be better suited (at least for the deployManager part of things) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The ZkCache just uses a Guava cache and we were hoping to get benefits from the LoadingCache created by CaffeineCache for debouncing requests because we were also getting timeouts at the DeployManager's request, do you still think that I should update the DeployManager's usage? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah ok, thought the Zk one was more similar. Can keep the separate one then |
||
private static final Logger LOG = LoggerFactory.getLogger(ManagerCache.class); | ||
|
||
public final boolean isEnabled; | ||
private final LoadingCache<K, V> cache; | ||
|
||
@Inject | ||
public ManagerCache(boolean isEnabled, int cacheTtl, Function<? super K, V> loader) { | ||
this.isEnabled = isEnabled; | ||
cache = | ||
Caffeine | ||
.newBuilder() | ||
.expireAfterWrite(cacheTtl, TimeUnit.SECONDS) | ||
.build(loader::apply); | ||
} | ||
|
||
public V get(K key) { | ||
V values = cache.get(key); | ||
if (values != null) { | ||
LOG.trace("Grabbed values for {} from cache", key); | ||
} else { | ||
LOG.trace("{} not in cache, setting", key); | ||
} | ||
|
||
return values; | ||
} | ||
|
||
public Map<K, V> getAll(@Nonnull Iterable<? extends K> keys) { | ||
Map<K, V> values = cache.getAll(keys); | ||
if (!values.isEmpty()) { | ||
LOG.trace("Grabbed mapped values for {} from cache", keys); | ||
} | ||
|
||
return values; | ||
} | ||
|
||
public boolean isEnabled() { | ||
return isEnabled; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,6 +90,7 @@ public class RequestManager extends CuratorAsyncManager { | |
); | ||
|
||
private final Map<Class<? extends SingularityExpiringRequestActionParent<? extends SingularityExpiringRequestParent>>, Transcoder<? extends SingularityExpiringRequestActionParent<? extends SingularityExpiringRequestParent>>> expiringTranscoderMap; | ||
private final ManagerCache<String, List<SingularityRequestWithState>> requestsCache; | ||
|
||
@Inject | ||
public RequestManager( | ||
|
@@ -133,6 +134,12 @@ public RequestManager( | |
|
||
this.leaderCache = leaderCache; | ||
this.webCache = webCache; | ||
this.requestsCache = | ||
new ManagerCache<>( | ||
configuration.useCaffeineCache(), | ||
configuration.getRequestCaffeineCacheTtl(), | ||
key -> this.fetchRequests() | ||
); | ||
} | ||
|
||
private String getRequestPath(String requestId) { | ||
|
@@ -632,11 +639,20 @@ public List<SingularityRequestWithState> getRequests(boolean useWebCache) { | |
if (useWebCache && webCache.useCachedRequests()) { | ||
return webCache.getRequests(); | ||
} | ||
|
||
if (requestsCache.isEnabled()) { | ||
List<SingularityRequestWithState> requests = requestsCache.get("all"); | ||
if (requests != null) { | ||
return requests; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we should just not use the web cache here if this is the case. Seems overkill to have the leaderCache -> web cache -> zk cache all here. Lots of duplicate memory usage to store it all 3 times There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed offline, to have ease in revert, the removal of web cache will be in a separate PR |
||
|
||
List<SingularityRequestWithState> requests = fetchRequests(); | ||
|
||
if (useWebCache) { | ||
webCache.cacheRequests(requests); | ||
} | ||
|
||
return requests; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit — can we remove
caffeine
from the config terminology here? Users don't need to be aware of cache choice/impl, and we could change caches in the future if we wanted to without any awkwardnessThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also updated the
useCaffeineCache
touseZKFastCache