Github user tumativ commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/673#discussion_r228896991
--- Diff:
zookeeper-server/src/main/java/org/apache/zookeeper/server/ServerCnxn.java ---
@@ -68,8 +68,39 @@
private volatile boolean stale = false;
+ AtomicLong outstandingCount = new AtomicLong();
+
+ /** The ZooKeeperServer for this connection. May be null if the server
+ * is not currently serving requests (for example if the server is not
+ * an active quorum participant.
+ */
+ final ZooKeeperServer zkServer;
+
+ public ServerCnxn(final ZooKeeperServer zkServer) {
+ this.zkServer = zkServer;
+ }
+
abstract int getSessionTimeout();
+ public void incrOutstandingAndCheckThrottle(RequestHeader h) {
+ if (h.getXid() <= 0) {
+ return;
+ }
+ if (zkServer.shouldThrottle(outstandingCount.incrementAndGet())) {
+ disableRecv(false);
+ }
+ }
+
+ // will be called from zkServer.processPacket
+ public void decrOutstandingAndCheckThrottle(ReplyHeader h) {
--- End diff --
I see adding multiple responsibilities into one unit like incrementing or
decrementing outstanding requests, checking the throttle and disabling/enabling
receive. They should always go together and sit together. The criteria of
disabling Recieve/ enabling can be changed at any time, not only based on the
just outstanding requests and throttle. It can also depend on other attributes
of the system. Why do not we inject the strategy into the server? One of the
strategies can be based on outstanding request and throttle. What do you think
exposing function like "canProcessRequest" which can internally check
strategies and respond?
---
