You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by enixon <gi...@git.apache.org> on 2018/10/31 21:05:07 UTC
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
GitHub user enixon opened a pull request:
https://github.com/apache/zookeeper/pull/684
ZOOKEEPER-3180: Add response cache to improve the throughput of read …
…heavy traffic
Introduces a ResponseCache that interacts with ServerCnxn to cache the serialized response for getData requests.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/enixon/zookeeper response-cache
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/684.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #684
----
commit 4e2f5a3ae0ef59b09fce9c835af13b5776d656c4
Author: Brian Nixon <ni...@...>
Date: 2018-10-31T19:11:54Z
ZOOKEEPER-3180: Add response cache to improve the throughput of read heavy traffic
Introduces a ResponseCache that interacts with ServerCnxn to cache the serialized response for getData requests.
----
---
[GitHub] zookeeper issue #684: ZOOKEEPER-3180: Add response cache to improve the thro...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on the issue:
https://github.com/apache/zookeeper/pull/684
@enixon
need another rebase .can we move on?
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r230973619
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/NIOServerCnxn.java ---
@@ -151,12 +148,17 @@ void sendBufferSync(ByteBuffer bb) {
* sendBuffer pushes a byte buffer onto the outgoing buffer queue for
* asynchronous writes.
*/
- public void sendBuffer(ByteBuffer bb) {
+ public void sendBuffer(ByteBuffer... buffers) {
if (LOG.isTraceEnabled()) {
LOG.trace("Add a buffer to outgoingBuffers, sk " + sk
+ " is valid: " + sk.isValid());
}
- outgoingBuffers.add(bb);
+ synchronized (outgoingBuffers) {
--- End diff --
This synchronization had to be added, because of the ability to send multiple byte buffers at once. What's the performance impact?
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r231199414
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/ResponseCache.java ---
@@ -0,0 +1,84 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.apache.jute.Record;
+import org.apache.zookeeper.data.Stat;
+
+@SuppressWarnings("serial")
+public class ResponseCache {
--- End diff --
How about updating the cache when watch event is triggered?
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r238789004
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/ResponseCache.java ---
@@ -0,0 +1,84 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.apache.jute.Record;
+import org.apache.zookeeper.data.Stat;
+
+@SuppressWarnings("serial")
+public class ResponseCache {
+ private static final int DEFAULT_RESPONSE_CACHE_SIZE = 400;
--- End diff --
Yes, I think we randomly chose one, it depends on how much data you have, we'll update the doc for the best practice. Also there is metric to show the cache hit rate, if it's too low, maybe we need to raise the cache size.
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r230973904
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/ResponseCache.java ---
@@ -0,0 +1,84 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.apache.jute.Record;
+import org.apache.zookeeper.data.Stat;
+
+@SuppressWarnings("serial")
+public class ResponseCache {
+ private static final int DEFAULT_RESPONSE_CACHE_SIZE = 400;
--- End diff --
Agreed. You might want to add some useful comment here.
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r238787735
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/DumbWatcher.java ---
@@ -69,7 +69,7 @@ public void sendCloseSession() { }
void setSessionId(long sessionId) { }
@Override
- void sendBuffer(ByteBuffer closeConn) { }
+ void sendBuffer(ByteBuffer... closeConn) { }
--- End diff --
In order to reduce the GC effort by re-using the cached serialized data, we need to break the single response packet into different part, the length buffer, the header buffer, and the data buffer, length and header will be built every time, but data buffer is reused, that's why we changed this to pass in a buffers list.
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r238790553
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/ResponseCache.java ---
@@ -0,0 +1,84 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.apache.jute.Record;
+import org.apache.zookeeper.data.Stat;
+
+@SuppressWarnings("serial")
+public class ResponseCache {
+ private static final int DEFAULT_RESPONSE_CACHE_SIZE = 400;
+
+ private static class Entry {
+ public Stat stat;
+ public byte[] data;
+ }
+
+ private Map<String, Entry> cache = Collections.synchronizedMap(
+ new LRUCache<String, Entry>(getResponseCacheSize()));
+
+ public ResponseCache() {
+ }
+
+ public void put(String path, byte[] data, Stat stat) {
+ Entry entry = new Entry();
+ entry.data = data;
+ entry.stat = stat;
+ cache.put(path, entry);
+ }
+
+ public byte[] get(String key, Stat stat) {
+ Entry entry = cache.get(key);
+ if (entry == null) {
+ return null;
+ }
+ if (!stat.equals(entry.stat)) {
+ // The node has been modified, invalidate cache.
+ cache.remove(key);
+ return null;
+ } else {
+ return entry.data;
+ }
+ }
+
+ private static int getResponseCacheSize() {
+ String value = System.getProperty("zookeeper.maxResponseCacheSize");
+ return value == null ? DEFAULT_RESPONSE_CACHE_SIZE : Integer.parseInt(value);
+ }
+
+ public static boolean isEnabled() {
+ return getResponseCacheSize() != 0;
+ }
+
+ private static class LRUCache<K, V> extends LinkedHashMap<K, V> {
--- End diff --
We can extend that, but it's better to be done in a separate thread, feel free to open a new JIRA to follow up on this.
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r231194896
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/ResponseCache.java ---
@@ -0,0 +1,84 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.apache.jute.Record;
+import org.apache.zookeeper.data.Stat;
+
+@SuppressWarnings("serial")
+public class ResponseCache {
+ private static final int DEFAULT_RESPONSE_CACHE_SIZE = 400;
+
+ private static class Entry {
+ public Stat stat;
+ public byte[] data;
+ }
+
+ private Map<String, Entry> cache = Collections.synchronizedMap(
--- End diff --
Would be good if DEFAULT_INITIAL_CAPACITY of cache also defined
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r238786172
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/NIOServerCnxn.java ---
@@ -151,12 +148,17 @@ void sendBufferSync(ByteBuffer bb) {
* sendBuffer pushes a byte buffer onto the outgoing buffer queue for
* asynchronous writes.
*/
- public void sendBuffer(ByteBuffer bb) {
+ public void sendBuffer(ByteBuffer... buffers) {
if (LOG.isTraceEnabled()) {
LOG.trace("Add a buffer to outgoingBuffers, sk " + sk
+ " is valid: " + sk.isValid());
}
- outgoingBuffers.add(bb);
+ synchronized (outgoingBuffers) {
--- End diff --
We have E2E perf regression detect for different use cases with different traffic, haven't seen any obvious perf impact there.
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r230973324
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/DumbWatcher.java ---
@@ -69,7 +69,7 @@ public void sendCloseSession() { }
void setSessionId(long sessionId) { }
@Override
- void sendBuffer(ByteBuffer closeConn) { }
+ void sendBuffer(ByteBuffer... closeConn) { }
--- End diff --
Why have you changed the serialization logic to use multiple buffers instead of a single one?
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r230973999
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/ResponseCache.java ---
@@ -0,0 +1,84 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.apache.jute.Record;
+import org.apache.zookeeper.data.Stat;
+
+@SuppressWarnings("serial")
+public class ResponseCache {
+ private static final int DEFAULT_RESPONSE_CACHE_SIZE = 400;
+
+ private static class Entry {
+ public Stat stat;
+ public byte[] data;
+ }
+
+ private Map<String, Entry> cache = Collections.synchronizedMap(
+ new LRUCache<String, Entry>(getResponseCacheSize()));
+
+ public ResponseCache() {
+ }
+
+ public void put(String path, byte[] data, Stat stat) {
+ Entry entry = new Entry();
+ entry.data = data;
+ entry.stat = stat;
+ cache.put(path, entry);
+ }
+
+ public byte[] get(String key, Stat stat) {
+ Entry entry = cache.get(key);
+ if (entry == null) {
+ return null;
+ }
+ if (!stat.equals(entry.stat)) {
--- End diff --
Comparing `mzxid` instead?
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r231110582
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/ResponseCache.java ---
@@ -0,0 +1,84 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.apache.jute.Record;
+import org.apache.zookeeper.data.Stat;
+
+@SuppressWarnings("serial")
+public class ResponseCache {
+ private static final int DEFAULT_RESPONSE_CACHE_SIZE = 400;
+
+ private static class Entry {
+ public Stat stat;
+ public byte[] data;
+ }
+
+ private Map<String, Entry> cache = Collections.synchronizedMap(
+ new LRUCache<String, Entry>(getResponseCacheSize()));
+
+ public ResponseCache() {
+ }
+
+ public void put(String path, byte[] data, Stat stat) {
+ Entry entry = new Entry();
+ entry.data = data;
+ entry.stat = stat;
+ cache.put(path, entry);
+ }
+
+ public byte[] get(String key, Stat stat) {
+ Entry entry = cache.get(key);
+ if (entry == null) {
+ return null;
+ }
+ if (!stat.equals(entry.stat)) {
+ // The node has been modified, invalidate cache.
+ cache.remove(key);
+ return null;
+ } else {
+ return entry.data;
+ }
+ }
+
+ private static int getResponseCacheSize() {
+ String value = System.getProperty("zookeeper.maxResponseCacheSize");
+ return value == null ? DEFAULT_RESPONSE_CACHE_SIZE : Integer.parseInt(value);
+ }
+
+ public static boolean isEnabled() {
+ return getResponseCacheSize() != 0;
+ }
+
+ private static class LRUCache<K, V> extends LinkedHashMap<K, V> {
--- End diff --
Can we provide the Eviction Policy like FIFO,LRU,LFU etc to cache and behave accordingly?
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r230974144
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/ResponseCache.java ---
@@ -0,0 +1,84 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.apache.jute.Record;
+import org.apache.zookeeper.data.Stat;
+
+@SuppressWarnings("serial")
+public class ResponseCache {
+ private static final int DEFAULT_RESPONSE_CACHE_SIZE = 400;
+
+ private static class Entry {
+ public Stat stat;
+ public byte[] data;
+ }
+
+ private Map<String, Entry> cache = Collections.synchronizedMap(
+ new LRUCache<String, Entry>(getResponseCacheSize()));
+
+ public ResponseCache() {
+ }
+
+ public void put(String path, byte[] data, Stat stat) {
+ Entry entry = new Entry();
+ entry.data = data;
+ entry.stat = stat;
+ cache.put(path, entry);
+ }
+
+ public byte[] get(String key, Stat stat) {
+ Entry entry = cache.get(key);
+ if (entry == null) {
+ return null;
+ }
+ if (!stat.equals(entry.stat)) {
+ // The node has been modified, invalidate cache.
+ cache.remove(key);
+ return null;
+ } else {
+ return entry.data;
+ }
+ }
+
+ private static int getResponseCacheSize() {
+ String value = System.getProperty("zookeeper.maxResponseCacheSize");
+ return value == null ? DEFAULT_RESPONSE_CACHE_SIZE : Integer.parseInt(value);
+ }
+
+ public static boolean isEnabled() {
+ return getResponseCacheSize() != 0;
+ }
+
+ private static class LRUCache<K, V> extends LinkedHashMap<K, V> {
+ private int cacheSize;
+
+ public LRUCache(int cacheSize) {
--- End diff --
I agreed with @hanm and like the idea of removing and re-putting nodes, but doesn't look like it can be easily implemented.
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r229938031
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/ServerMetrics.java ---
@@ -67,7 +67,10 @@
SNAP_COUNT(new SimpleCounter("snap_count")),
COMMIT_COUNT(new SimpleCounter("commit_count")),
CONNECTION_REQUEST_COUNT(new SimpleCounter("connection_request_count")),
- BYTES_RECEIVED_COUNT(new SimpleCounter("bytes_received_count"));
+ BYTES_RECEIVED_COUNT(new SimpleCounter("bytes_received_count")),
+
+ RESPONSE_PACKET_CACHE_HITS(new SimpleCounter("response_packet_cache_hits")),
+ RESPONSE_PACKET_CACHE_MISSING(new SimpleCounter("response_packet_cache_misses"));
--- End diff --
how well this cache works under different type of workloads, based on the stats here? (if it's ok to share)
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r229934517
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/ResponseCache.java ---
@@ -0,0 +1,84 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.apache.jute.Record;
+import org.apache.zookeeper.data.Stat;
+
+@SuppressWarnings("serial")
+public class ResponseCache {
+ private static final int DEFAULT_RESPONSE_CACHE_SIZE = 400;
+
+ private static class Entry {
+ public Stat stat;
+ public byte[] data;
+ }
+
+ private Map<String, Entry> cache = Collections.synchronizedMap(
+ new LRUCache<String, Entry>(getResponseCacheSize()));
+
+ public ResponseCache() {
+ }
+
+ public void put(String path, byte[] data, Stat stat) {
+ Entry entry = new Entry();
+ entry.data = data;
+ entry.stat = stat;
+ cache.put(path, entry);
+ }
+
+ public byte[] get(String key, Stat stat) {
+ Entry entry = cache.get(key);
+ if (entry == null) {
+ return null;
+ }
+ if (!stat.equals(entry.stat)) {
+ // The node has been modified, invalidate cache.
+ cache.remove(key);
+ return null;
+ } else {
+ return entry.data;
+ }
+ }
+
+ private static int getResponseCacheSize() {
+ String value = System.getProperty("zookeeper.maxResponseCacheSize");
+ return value == null ? DEFAULT_RESPONSE_CACHE_SIZE : Integer.parseInt(value);
+ }
+
+ public static boolean isEnabled() {
+ return getResponseCacheSize() != 0;
+ }
+
+ private static class LRUCache<K, V> extends LinkedHashMap<K, V> {
--- End diff --
nice.
---
[GitHub] zookeeper issue #684: ZOOKEEPER-3180: Add response cache to improve the thro...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/684
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2552/
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r229934430
--- Diff: zookeeper-server/src/test/java/org/apache/zookeeper/test/ResponseCacheTest.java ---
@@ -0,0 +1,118 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.test;
+
+import java.util.Map;
+
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.data.Stat;
+import org.apache.zookeeper.server.ServerStats;
--- End diff --
unused import?
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r229936526
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/ResponseCache.java ---
@@ -0,0 +1,84 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.apache.jute.Record;
+import org.apache.zookeeper.data.Stat;
+
+@SuppressWarnings("serial")
+public class ResponseCache {
+ private static final int DEFAULT_RESPONSE_CACHE_SIZE = 400;
+
+ private static class Entry {
+ public Stat stat;
+ public byte[] data;
+ }
+
+ private Map<String, Entry> cache = Collections.synchronizedMap(
+ new LRUCache<String, Entry>(getResponseCacheSize()));
+
+ public ResponseCache() {
+ }
+
+ public void put(String path, byte[] data, Stat stat) {
+ Entry entry = new Entry();
+ entry.data = data;
+ entry.stat = stat;
+ cache.put(path, entry);
+ }
+
+ public byte[] get(String key, Stat stat) {
+ Entry entry = cache.get(key);
+ if (entry == null) {
+ return null;
+ }
+ if (!stat.equals(entry.stat)) {
+ // The node has been modified, invalidate cache.
+ cache.remove(key);
+ return null;
+ } else {
+ return entry.data;
+ }
+ }
+
+ private static int getResponseCacheSize() {
+ String value = System.getProperty("zookeeper.maxResponseCacheSize");
--- End diff --
would be good to put this in document, but this can be done in a different jira if you like.
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r229934995
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/ResponseCache.java ---
@@ -0,0 +1,84 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.apache.jute.Record;
+import org.apache.zookeeper.data.Stat;
+
+@SuppressWarnings("serial")
+public class ResponseCache {
+ private static final int DEFAULT_RESPONSE_CACHE_SIZE = 400;
+
+ private static class Entry {
+ public Stat stat;
+ public byte[] data;
+ }
+
+ private Map<String, Entry> cache = Collections.synchronizedMap(
+ new LRUCache<String, Entry>(getResponseCacheSize()));
+
+ public ResponseCache() {
+ }
+
+ public void put(String path, byte[] data, Stat stat) {
+ Entry entry = new Entry();
+ entry.data = data;
+ entry.stat = stat;
+ cache.put(path, entry);
+ }
+
+ public byte[] get(String key, Stat stat) {
+ Entry entry = cache.get(key);
+ if (entry == null) {
+ return null;
+ }
+ if (!stat.equals(entry.stat)) {
+ // The node has been modified, invalidate cache.
+ cache.remove(key);
+ return null;
+ } else {
+ return entry.data;
+ }
+ }
+
+ private static int getResponseCacheSize() {
+ String value = System.getProperty("zookeeper.maxResponseCacheSize");
+ return value == null ? DEFAULT_RESPONSE_CACHE_SIZE : Integer.parseInt(value);
+ }
+
+ public static boolean isEnabled() {
+ return getResponseCacheSize() != 0;
+ }
+
+ private static class LRUCache<K, V> extends LinkedHashMap<K, V> {
+ private int cacheSize;
+
+ public LRUCache(int cacheSize) {
--- End diff --
This constructor will use insertion order, instead of access order. So for cache entries that's most recently queried it's possible these entries get ejected if they were inserted long time ago, when cache capacity is full. I am curious, what's the trade off you made to decide to use insertion order, instead of access order?
One improvement that could be made is in `ResponseCache.get`, when we have a cache hit, we always remove the entry and re-put the same entry, to keep the most recently queried order up to date.
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r238788593
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/ResponseCache.java ---
@@ -0,0 +1,84 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.apache.jute.Record;
+import org.apache.zookeeper.data.Stat;
+
+@SuppressWarnings("serial")
+public class ResponseCache {
--- End diff --
Since this is a read cache, it's better to invalidate it when the client read the changed data to avoid unnecessary update.
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r238794022
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/ResponseCache.java ---
@@ -0,0 +1,84 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.apache.jute.Record;
+import org.apache.zookeeper.data.Stat;
+
+@SuppressWarnings("serial")
+public class ResponseCache {
+ private static final int DEFAULT_RESPONSE_CACHE_SIZE = 400;
+
+ private static class Entry {
+ public Stat stat;
+ public byte[] data;
+ }
+
+ private Map<String, Entry> cache = Collections.synchronizedMap(
+ new LRUCache<String, Entry>(getResponseCacheSize()));
+
+ public ResponseCache() {
+ }
+
+ public void put(String path, byte[] data, Stat stat) {
+ Entry entry = new Entry();
+ entry.data = data;
+ entry.stat = stat;
+ cache.put(path, entry);
+ }
+
+ public byte[] get(String key, Stat stat) {
+ Entry entry = cache.get(key);
+ if (entry == null) {
+ return null;
+ }
+ if (!stat.equals(entry.stat)) {
--- End diff --
Currently, he stat is also part of the cached data, when it's changed we need to invalidate this as well.
We can separate the cache for data and stat, but we didn't see that's necessary for now, this is mainly for improving the serializing performance and GC effort with large znode data size.
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r230973727
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/NIOServerCnxn.java ---
@@ -235,10 +237,12 @@ void handleWrite(SelectionKey k) throws IOException, CloseRequestException {
if (bb == ServerCnxnFactory.closeConn) {
throw new CloseRequestException("close requested");
}
+ if (bb == packetSentinel) {
--- End diff --
Seems like you're fixing a bug by introducing `packetSentinel`. Is that correct?
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r230975652
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/ServerCnxn.java ---
@@ -68,29 +70,74 @@
private volatile boolean stale = false;
+ final ZooKeeperServer zkServer;
+
+ public ServerCnxn(final ZooKeeperServer zkServer) {
+ this.zkServer = zkServer;
+ }
+
abstract int getSessionTimeout();
abstract void close();
+ public abstract void sendResponse(ReplyHeader h, Record r,
+ String tag, String cacheKey, Stat stat) throws IOException;
+
public void sendResponse(ReplyHeader h, Record r, String tag) throws IOException {
- ByteArrayOutputStream baos = new ByteArrayOutputStream();
- // Make space for length
+ sendResponse(h, r, tag, null, null);
+ }
+
+ protected byte[] serializeRecord(Record record) throws IOException {
+ ByteArrayOutputStream baos = new ByteArrayOutputStream(
+ ZooKeeperServer.intBufferStartingSizeBytes);
BinaryOutputArchive bos = BinaryOutputArchive.getArchive(baos);
- try {
- baos.write(fourBytes);
- bos.writeRecord(h, "header");
- if (r != null) {
- bos.writeRecord(r, tag);
+ bos.writeRecord(record, null);
+ return baos.toByteArray();
+ }
+
+ protected ByteBuffer[] serialize(ReplyHeader h, Record r, String tag,
+ String cacheKey, Stat stat) throws IOException {
+ byte[] header = serializeRecord(h);
+ byte[] data = null;
+ if (r != null) {
+ ResponseCache cache = zkServer.getReadResponseCache();
+ if (cache != null && stat != null && cacheKey != null &&
+ !cacheKey.endsWith(Quotas.statNode)) {
+ // Use cache to get serialized data.
+ //
+ // NB: Tag is ignored both during cache lookup and serialization,
+ // since is is not used in read responses, which are being cached.
+ data = cache.get(cacheKey, stat);
+ if (data == null) {
+ // Cache miss, serialize the response and put it in cache.
+ data = serializeRecord(r);
--- End diff --
`put()` aquires the write lock anyway, so in my opinion this will cause the response to be serialized twice and put into the cache twice (second overwrites first). I think it's fine.
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r231189992
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/ResponseCache.java ---
@@ -0,0 +1,84 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.apache.jute.Record;
+import org.apache.zookeeper.data.Stat;
+
+@SuppressWarnings("serial")
+public class ResponseCache {
+ private static final int DEFAULT_RESPONSE_CACHE_SIZE = 400;
+
+ private static class Entry {
+ public Stat stat;
+ public byte[] data;
+ }
+
+ private Map<String, Entry> cache = Collections.synchronizedMap(
--- End diff --
Can LRUcache compose the LinkedHashMap rather extending? Can we use read-write locks for better synchronization? The read lock on reads will give better results rather locking on the map.
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r229935808
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/ResponseCache.java ---
@@ -0,0 +1,84 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.apache.jute.Record;
+import org.apache.zookeeper.data.Stat;
+
+@SuppressWarnings("serial")
+public class ResponseCache {
+ private static final int DEFAULT_RESPONSE_CACHE_SIZE = 400;
--- End diff --
Do we randomly choose 400 as default? Any recommendations on how to choose a good default cache size :) ?
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r238788174
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/NIOServerCnxn.java ---
@@ -235,10 +237,12 @@ void handleWrite(SelectionKey k) throws IOException, CloseRequestException {
if (bb == ServerCnxnFactory.closeConn) {
throw new CloseRequestException("close requested");
}
+ if (bb == packetSentinel) {
--- End diff --
Since we separate the response into multiple pieces, length, header, data, we need a way to tell if it's the end of a response or not, and the sentinel is added for this purpose.
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r229871456
--- Diff: zookeeper-server/src/test/java/org/apache/zookeeper/test/ResponseCacheTest.java ---
@@ -0,0 +1,118 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.test;
+
+import java.util.Map;
+
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.data.Stat;
+import org.apache.zookeeper.server.ServerStats;
+import org.apache.zookeeper.server.ServerMetrics;
+import org.apache.zookeeper.server.ZooKeeperServerMXBean;
+import org.junit.Assert;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.management.JMX;
+import javax.management.MBeanServerConnection;
+import javax.management.ObjectName;
+
+public class ResponseCacheTest extends ClientBase {
+ protected static final Logger LOG =
+ LoggerFactory.getLogger(ResponseCacheTest.class);
+
+ @Test
+ public void testResponseCache() throws Exception {
+ ObjectName bean = JMXEnv.getServerBean();
+ MBeanServerConnection mbsc = JMXEnv.conn();
+ ZooKeeperServerMXBean zkBean = JMX.newMBeanProxy(mbsc, bean, ZooKeeperServerMXBean.class);
+ ZooKeeper zk = createClient();
+
+ try {
+ performCacheTest(zk, zkBean, "/cache", true);
+ performCacheTest(zk, zkBean, "/nocache", false);
+ }
+ finally {
+ zk.close();
+ }
+ }
+
+ private void checkCacheStatus(long expectedHits, long expectedMisses) {
+ Map<String, Long> metrics = ServerMetrics.getAllValues();
+ Assert.assertEquals((Long) expectedHits, metrics.get("response_packet_cache_hits"));
+ Assert.assertEquals((Long) expectedMisses, metrics.get("response_packet_cache_misses"));
+ }
+
+ public void performCacheTest(ZooKeeper zk, ZooKeeperServerMXBean zkBean, String path, boolean useCache) throws Exception {
+ ServerMetrics.resetAll();
+ Stat writeStat = new Stat();
+ Stat readStat = new Stat();
+ byte[] readData = null;
+ int reads = 10;
+ long expectedHits = 0;
+ long expectedMisses = 0;
+
+ zkBean.setResponseCachingEnabled(useCache);
--- End diff --
Brian, looks like we call ZooKeeperServer.setResponseCachingEnabled directly instead of introducing the JMX bean here, can you check if we can get rid of the JMX bean change in this diff?
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r229937485
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/ResponseCache.java ---
@@ -0,0 +1,84 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.apache.jute.Record;
+import org.apache.zookeeper.data.Stat;
+
+@SuppressWarnings("serial")
+public class ResponseCache {
+ private static final int DEFAULT_RESPONSE_CACHE_SIZE = 400;
+
+ private static class Entry {
+ public Stat stat;
+ public byte[] data;
+ }
+
+ private Map<String, Entry> cache = Collections.synchronizedMap(
+ new LRUCache<String, Entry>(getResponseCacheSize()));
+
+ public ResponseCache() {
+ }
+
+ public void put(String path, byte[] data, Stat stat) {
+ Entry entry = new Entry();
+ entry.data = data;
+ entry.stat = stat;
+ cache.put(path, entry);
+ }
+
+ public byte[] get(String key, Stat stat) {
+ Entry entry = cache.get(key);
+ if (entry == null) {
+ return null;
+ }
+ if (!stat.equals(entry.stat)) {
--- End diff --
stat could change even the data stored in a znode does not, such creating / deleting children of a znode will change its stat's pzxid and the cversion... so we will have a cache miss in this case even the data (that we are interested in for `getData` response) does not. Is this expected? It seems more intuitive to get a cache hit in this case.
---
[GitHub] zookeeper pull request #684: ZOOKEEPER-3180: Add response cache to improve t...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/684#discussion_r229870463
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/ServerCnxn.java ---
@@ -68,29 +70,74 @@
private volatile boolean stale = false;
+ final ZooKeeperServer zkServer;
+
+ public ServerCnxn(final ZooKeeperServer zkServer) {
+ this.zkServer = zkServer;
+ }
+
abstract int getSessionTimeout();
abstract void close();
+ public abstract void sendResponse(ReplyHeader h, Record r,
+ String tag, String cacheKey, Stat stat) throws IOException;
+
public void sendResponse(ReplyHeader h, Record r, String tag) throws IOException {
- ByteArrayOutputStream baos = new ByteArrayOutputStream();
- // Make space for length
+ sendResponse(h, r, tag, null, null);
+ }
+
+ protected byte[] serializeRecord(Record record) throws IOException {
+ ByteArrayOutputStream baos = new ByteArrayOutputStream(
+ ZooKeeperServer.intBufferStartingSizeBytes);
BinaryOutputArchive bos = BinaryOutputArchive.getArchive(baos);
- try {
- baos.write(fourBytes);
- bos.writeRecord(h, "header");
- if (r != null) {
- bos.writeRecord(r, tag);
+ bos.writeRecord(record, null);
+ return baos.toByteArray();
+ }
+
+ protected ByteBuffer[] serialize(ReplyHeader h, Record r, String tag,
+ String cacheKey, Stat stat) throws IOException {
+ byte[] header = serializeRecord(h);
+ byte[] data = null;
+ if (r != null) {
+ ResponseCache cache = zkServer.getReadResponseCache();
+ if (cache != null && stat != null && cacheKey != null &&
+ !cacheKey.endsWith(Quotas.statNode)) {
+ // Use cache to get serialized data.
+ //
+ // NB: Tag is ignored both during cache lookup and serialization,
+ // since is is not used in read responses, which are being cached.
+ data = cache.get(cacheKey, stat);
+ if (data == null) {
+ // Cache miss, serialize the response and put it in cache.
+ data = serializeRecord(r);
--- End diff --
We may hit the race condition to serialize the same record multiple times, but we made a trade off, instead of having write lock every time, this might be more efficient.
---