zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From tumativ <...@git.apache.org>
Subject [GitHub] zookeeper pull request #673: [ZOOKEEPER-3177] Refactor request throttle logi...
Date Mon, 29 Oct 2018 12:00:43 GMT
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?


---

Mime
View raw message