hanm commented on issue #1098: ZOOKEEPER-3560: Add response cache to serve get
children (2) requests.
URL: https://github.com/apache/zookeeper/pull/1098#issuecomment-547210748
>> serialization is a cpu-cost operation?
Serialization is not just CPU bound; it's also memory bound. Most JVM uses a
generational garbage collector where it treats objects on heap differently
based on their age. The response objects (and the reachable objects like
buffers) are very short lived, the serialization code path will create many
such short lived objects through allocations from Eden space. When QPS is high,
the Eden (and survivor) spaces will be filled up much faster, forced JVM to
promote these short lived objects to the older generation at a young age than
the normal tenuring threshold, which could lead to excessive fragmentation of
the old generation, and this creates all sorts of problems for the actual GC
(CMS, for example), such as increased GC time and sometimes full GC.
>> Putting more thing into the cache(memory) will hava more likely to
trigger the full GC(STW)?
Yes, if not choosing properly, caching would lead to more memory pressure as
this cache feature essentially reduces the available heap space. But, when
choosing properly, this feature will benefit both request serving, and improve
GC.
>> I also doubt about: adding this cache will bring about some issues about
read inconsistency
We invalidate cache when the cached data (zNode) changed; I don't see a
particular consistency issue here. The sequential read consistency is still
guaranteed. If you think there is a read inconsistency issue, do you mind
providing a concrete example to illustrate the problem?
>> I also do a benchmark for this feature
Nice work, thanks for spending time doing that.
I want to point out that writing a benchmark for ZooKeeper (and in general,
for any non trivial systems) is hard - as there are too many variables in the
equation. For your benchmark, did you measure other metrics (request latency,
gc/msec/cycles) with more types of workload (e.g. more read heavy - 1k per sec
is nothing for ZK, try 50k or 100k) with different sizes of cache (at least, we
should measure 400, which is the default value, instead of 1000)... and this
list can go on and on. Of course, we have to respect the existing insights the
(limited) benchmark provides, but it would be premature to conclude how the
feature would perform without benchmark all dimensions extensively.
An extensive benchmark could provide a great deal of insights on performance
characteristics of a system under different workloads and configurations, but
the real test is on production environment. This caching feature works great
for the clusters I operate and I expect similar success stories from @enixon
who contributed the original feature.
>> it should be enable by default?
I'd like to have the feature enable by default based on the observations and
experience of running this feature on some of our production cluster with
default value with a variety of workloads. Disabling such a feature will be
sad, considering lots of ZooKeeper users might not even aware of such a feature
due to the monstrous amount of configuration options of ZooKeeper. That said, I
am flexible on disabling the caching feature by default if community consent
so, in which case I'd propose that being done in a different JIRA consider this
pull request is ready to land.
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:
us...@infra.apache.org
With regards,
Apache Git Services
