[
https://issues.apache.org/jira/browse/HBASE-13170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14382672#comment-14382672
]
Sean Busbey commented on HBASE-13170:
-------------------------------------
This looks great. Could you add a general description and configuration info to
the ref guide? Right now the section on the block cache that talks about
getting things off the java heap talks about the L2 cache and BucketCache.
Some additional feedback below.
{code}
+ <dependency>
+ <groupId>net.spy</groupId>
+ <artifactId>spymemcached</artifactId>
+ </dependency>
{code}
Can we flag this as optional, with a comment that it's needed for the external
memcached blockcache?
The discussion above sounds like we don't expect the memcached cache to be the
common case, and flagging it optional means it won't show up as a transitive
dependency (i.e. when folks depend on hbase-server for the MapReduce bindings).
{code}
- } catch(InterruptedException e) {}
+ } catch(InterruptedException e) {
+ LOG.warn("Interrupted eviction thread ", e);
+ }
{code}
Also reset the thread's interrupted status.
{code}
+ private static BlockCache getExternalBlockcache(Configuration c) {
+ try {
+ Class klass = c.getClass(EXTERNAL_BLOCKCACHE_CLASS_KEY,
MemcachedBlockCache.class);
+ LOG.info("Creating external block cache of type: " + klass);
+ return (BlockCache) ReflectionUtils.newInstance(klass, c);
+ } catch (Exception e) {
+ LOG.warn("Error creating external block cache", e);
+ }
{code}
Could we use an enum of external blockcache implementations that falls back to
using class names? That way we can avoid labeling MemcachedBlockCache as
IO.Limited(CONFIG) because folks would just use a short name (e.g. "memcached").
There's an [example over in the WALFactory
code|https://github.com/apache/hbase/blob/master/hbase-server/src/main/java/org/apache/hadoop/hbase/wal/WALFactory.java#L132].
{code}
[email protected]
+public class MemcachedBlockCache implements BlockCache {
{code}
This should probably be IA.LimitedPrivate(CONFIG), unless we go with the enum.
{code}
+ LOG.info("Creating MemcachedBlockCache");
+ ConnectionFactoryBuilder builder = new ConnectionFactoryBuilder()
+ .setOpTimeout(TimeUnit.SECONDS.toMillis(1))
+ .setOpQueueMaxBlockTime(1500) // Cap the max time before
anything times out
{code}
I hate configuration knobs, but should this be one?
{code}
+ // Catch a pretty broad set of exceptions to limit any changes in the
memecache client
+ // and how it handles failures from leaking into the read path.
+ LOG.warn("Exception pulling from memcached [ " + cacheKey.toString() + "
]", e);
{code}
Please update the message to say e.g. that we're going to treat it as a cache
miss and continue.
Can we just include the exception message at WARN and move the stack trace to
debug?
{code}
+ @Override
+ public boolean evictBlock(BlockCacheKey cacheKey) {
+ try {
+ cacheStats.evict();
+ return client.delete(cacheKey.toString()).get();
+ } catch (InterruptedException e) {
+ LOG.warn("Error deleting " + cacheKey.toString(), e);
+ } catch (ExecutionException e) {
+ LOG.warn("Error deleting " + cacheKey.toString(), e);
+ }
+ return false;
+ }
{code}
In the interrupted exception block, please set the thread's interrupted status.
Can we move these stack traces to debug while keeping the general error and
message at warn?
{code}
diff --git
a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/PressureAwareCompactionThroughputController.java
b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/PressureAwareCompactionThroughputController.java
index 11ab568..5b079b0 100644
---
a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/PressureAwareCompactionThroughputController.java
+++
b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/PressureAwareCompactionThroughputController.java
@@ -198,6 +198,10 @@ public class PressureAwareCompactionThroughputController
extends Configured impl
@Override
public long control(String compactionName, long size) throws
InterruptedException {
ActiveCompaction compaction = activeCompactions.get(compactionName);
+ if (compaction == null) {
+ return 0;
+ }
+
compaction.totalSize += size;
long deltaSize = compaction.totalSize - compaction.lastControlSize;
if (deltaSize < controlPerSize) {
@@ -235,12 +239,14 @@ public class PressureAwareCompactionThroughputController
extends Configured impl
@Override
public void finish(String compactionName) {
ActiveCompaction compaction = activeCompactions.remove(compactionName);
- long elapsedTime = Math.max(1, EnvironmentEdgeManager.currentTime() -
compaction.startTime);
- LOG.info(compactionName + " average throughput is "
- + throughputDesc(compaction.totalSize, elapsedTime) + ", slept "
- + compaction.numberOfSleeps + " time(s) and total slept time is "
- + compaction.totalSleepTime + " ms. " + activeCompactions.size()
- + " active compactions remaining, total limit is " +
throughputDesc(maxThroughput));
+ if (compaction != null) {
+ long elapsedTime = Math.max(1, EnvironmentEdgeManager.currentTime() -
compaction.startTime);
+ LOG.info(compactionName + " average throughput is "
+ + throughputDesc(compaction.totalSize, elapsedTime) + ", slept "
+ + compaction.numberOfSleeps + " time(s) and total slept time is "
+ + compaction.totalSleepTime + " ms. " + activeCompactions.size()
+ + " active compactions remaining, total limit is " +
throughputDesc(maxThroughput));
+ }
}
private volatile boolean stopped = false;
{code}
I think these changes aren't related, could we move them to a different ticket?
> Allow block cache to be external
> --------------------------------
>
> Key: HBASE-13170
> URL: https://issues.apache.org/jira/browse/HBASE-13170
> Project: HBase
> Issue Type: New Feature
> Components: io
> Affects Versions: 2.0.0
> Reporter: Elliott Clark
> Assignee: Elliott Clark
> Fix For: 1.1.0
>
> Attachments: HBASE-13170-v1.patch, HBASE-13170-v2.patch,
> HBASE-13170-v3.patch, HBASE-13170.patch
>
>
> Allow an external service to provide the block cache. This has the nice
> property of allowing failover/upgrades to happen without causing a fully cold
> cache.
> Additionally this allows read replicas to share some of the same memory.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
