[
https://issues.apache.org/jira/browse/HBASE-5349?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13789131#comment-13789131
]
Anoop Sam John commented on HBASE-5349:
---------------------------------------
{quote}
I can see making the Interface public but do the implementations have to be
public too?
-public class LruBlockCache implements BlockCache, HeapSize {
+public class LruBlockCache implements ResizableBlockCache, HeapSize {
They are not instantiated outside of the hfile package?
{quote}
You mean LruBlockCache to be public or not right? It was already public. I can
see it is being referred in some test cases outside hfile package
bq.Call it DefaultHeapMemoryBalancer instead of DefaultHeapMemoryBalancerImpl I
would say.
Done
bq.Nit: does the context need both blocked and unblocked? + boolean
memstoreSufficient = blockedFlushCount == 0 && unblockedFlushCount == 0;
The current impl of the Tuner does not distinguish this. But Why I added it as
2 counts is to support more Tuner impl work (later?) Right now when both
memstore and block cache is under preassure we wont do any tuning. May be there
also some tuning can be done if blockedFlushCount is large. Blocked flush
blocks the writes to memstore.. Just to keep the door open I made this way :)
bq.You have this comment twice '// Increase the block cache size and
corresponding decrease in memstore size' One must be wrong (smile)
Copy paste.. :) Corrected
bq.Reading DefaultHeapMemoryBalancerImpl, we keep stepping w/o regard for a
max. Is max enforced elsewhere? If so, will DefaultHeapMemoryBalancerImpl keep
asking to step though we are against the max? Maybe this is fine. It makes the
implementation 'easier' which is good. Should we log when we want to 'step'? Or
is that done outside of DefaultHeapMemoryBalancerImpl (which would probably be
better... again keep it simple)
Yes I was intended to add check in HeapMemoryTuner#chore(). (The logging also).
Adding it now. Also added check in DefaultHeapMemoryBalancer itself. When the
HeapMemoryBalancer asks for step but the max limit is reached, I am making a
warn log now. May be I can change this to INFO level (?) IMO logging is enough
no other action is needed. Any suggestion? ( This is there in Ted's comment
also)
{quote}
Does the HeapMemoryManager have to have these two members?
+ private final MemStoreFlusher memStoreFlusher;
+ private final HRegionServer hrs;
Can it not take Interface that has just what it needs? Else makes it hard to
test this new code in isolation. At a minimum the HRS can be replaces by
Service or RegionServerService Interface? Is that possible? And FlushRequester
instead of MemStoreFlusher?
{quote}
Good point. Thanks I am making hrs to be of type Server which is enough.
private final Server server;
private final FlushRequester memStoreFlusher;
bq.Perhaps drop the 'Auto' prefix from AutoTunerContext and AutoTunerResult.
The users of these Interfaces don't care if it Auto or not. Ditto here:
HeapMemoryAutoTuner... call it HeapMemoryTuner.
Done
{quote}
These should be private since server side?
[email protected]
[email protected]
+public interface HeapMemoryBalancer
{quote}
I wanted the impl of this balancer to be pluggable so that users can impl there
own way like LoadBalancer. LoadBalancer marked as @InterfaceAudience.Public.
Just followed that. As it is server side we need to make private? I am not
sure of the guideline for this. If this is to be private I can change NP.
bq.Should there be a kill switch for the tuner? You pass in absolute values and
once set, it stops balancing (for the case were the tuner turns pathological
and an operator wants to turn it off while the server is online). We could do
that in another issue np.
Yes very much. This is needed. I have identified it as a subtask in my earlier
comment (6. Support turn ON/OFF the memory tuner at run time.) This is not done
yet. May be once this is done, as another issue or sub task this can be done.
bq.These do not need to be public classes + public static final class
AutoTunerContext { and AutoTunerResult?
This Context is passed to the HeapMemoryBalancer impl (which can be pluggable)
and it needs to return the result. Making the class non public will force the
impl can be to in the same package. Is that fine? That is why I kept it public
{quote}
This seems like informational rather than a warn?
+ LOG.warn("Current heap configuration from HeapMemoryBalancer exceeds "
+ + "the threshold required for successful cluster operation. "
+ + "The combined value cannot exceed 0.8. " + MemStoreFlusher.MEMSTORE_SIZE_KEY
+ + " is " + memstoreSize + " and " + HConstants.HFILE_BLOCK_CACHE_SIZE_KEY + "
is "
+ + blockCacheSize);
{quote}
Fine. INFO
bq.Do you need to shut it down? + startHeapMemoryManager(); Or it doesn't
matter?
It doen't matter as of now as the balancer thread is daemon. But I will add
shutdown also and call from HRS. May be latter when this is necessary it will a
place holder to add those shutdown logic. You fine with such a place holder?
(Only some log will be there as of now)
{quote}
Does the interface need to be public
+public interface MemstoreFlushListener {
{quote}
Done
bq.Needs tests. Hopefully you can change the above references to
MemstoreFlusher and RegionServer to be Interfaces so you do not need to spin up
a cluster to test (you are almost there).
Added tests
bq.I am a big fan of this patch.
Me too wanted this feature. Atleast some people asked me regarding this. What
they wanted was during some time, the cluster will be write heavy but no reads.
And later only reads but no writes.
{quote}
Make one log rather than two since they change in lockstep:
+ LOG.info("Setting block cache heap size to " + newBlockCacheSize);
...
+ LOG.info("Setting memstore heap size to " + newMemstoreSize);
{quote}
Done
bq.Should there be a 'damping' facility? That is, should we run the check more
often and only make changes if we have been called ten times and on 8 or the 10
times, we judged we should 'step'? That could be a different implementation I
suppose? Or conversely, do you think there should be an 'emergency' chain that
can be pulled when we need to change the configs now? (This latter is probably
not a good idea – at least not yet).
Thought abt that Stack. Some thing like a rule based decision making. The 1st
impl wanted to make simple. That is why making the decision maker pluggable so
that we can give better impls like what we have done in LoadBalancer.
bq.There're ^M's at the end of each line. Please generate patch on Linux.
Hope it is fine now
{quote}
For HeapMemoryAutoTuner:
+ AutoTunerContext context = createTunerContext();^M
Do we need to create new context for each iteration of the chore ? Can one
instance be reused ?
We need the the context as state var. Ya it should be okey. Done
+ if (1 - (memstoreSize + blockCacheSize) <
HConstants.HBASE_CLUSTER_MINIMUM_MEMORY_THRESHOLD) {^M
+ LOG.warn("Current heap configuration from HeapMemoryBalancer exceeds
"^M
+ + "the threshold required for successful cluster operation. "^M
Should some action be taken for the above case ? Otherwise tuning is
effectively disabled.
{quote}
We can not turn beyond this level as it will affect the normal functioning of
the RS. What action you think? Actions like abort and all like over kill.
{quote}
+ LOG.debug("From HeapMemoryBalancer new memstoreSize: " + memstoreSize
+ "%. new blockCacheSize: " + blockCacheSize + "%");^M
I think the percent sign is not needed.
{quote}
Converting to % for correct comparison now. So adding % is fine?
bq.For DefaultHeapMemoryBalancerImpl, please add javadoc and audience
annotation.
Done
bq.+ result.setMemstoreSize(context.getCurMemStoreSize() - step);
Should we check that the decrement would not produce negative result ?
Done.
> Automagically tweak global memstore and block cache sizes based on workload
> ---------------------------------------------------------------------------
>
> Key: HBASE-5349
> URL: https://issues.apache.org/jira/browse/HBASE-5349
> Project: HBase
> Issue Type: Improvement
> Affects Versions: 0.92.0
> Reporter: Jean-Daniel Cryans
> Assignee: Anoop Sam John
> Attachments: HBASE-5349_V2.patch, WIP_HBASE-5349.patch
>
>
> Hypertable does a neat thing where it changes the size given to the CellCache
> (our MemStores) and Block Cache based on the workload. If you need an image,
> scroll down at the bottom of this link:
> http://www.hypertable.com/documentation/architecture/
> That'd be one less thing to configure.
--
This message was sent by Atlassian JIRA
(v6.1#6144)
