Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[issue][improve] zookeeper connections monitor data #9778

Closed
wants to merge 1 commit into from

Conversation

xiaotongwang1
Copy link
Contributor

Fixes #9777

Motivation

The value of zookeeper_server_connections is incorrect when we open tls connection between client(broker、bookie) and zookeeper server

Modifications

when zkServer.getSecureServerCnxnFactory() not null ,will plus the connections and return

ServerCnxnFactory secCnxFactory = zkServer.getSecureServerCnxnFactory();
if (secCnxFactory != null) {
connections += secCnxFactory.getNumAliveConnections();
}

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations:(no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

  • Does this pull request introduce a new feature?(no)

…is incorrect when we open tls connection between client(broker、bookie) and zookeeper server

Changes: return the zookeeper connections both secure and non-secure
@merlimat
Copy link
Contributor

merlimat commented Mar 2, 2021

This fix is fine, though since we have moved to ZK 3.6, we should drop the existing way of gathering the ZK metrics using AOP interceptors and instead enable the supported Prometheus metrics that ZK provides.

@xiaotongwang1
Copy link
Contributor Author

@merlimat yes ,agree. I check the zookeeper state code ,it register a key with "num_alive_connections" ,and current AOP register a key with "zookeeper_server_connections",i think it can support export both 8000(current AOP style) and 7000 (zookeeper 3.6.x) next one or two release version ,then change the grafana template with num_alive_connections、watch_count、ephemerals_count..... and pulsar-zookeeper ZooKeeperStarter

============code of zookeeper 3.6.x=============
/**
* return the total number of client connections that are alive
* to this server
*/
public int getNumAliveConnections() {
int numAliveConnections = 0;

    if (serverCnxnFactory != null) {
        numAliveConnections += serverCnxnFactory.getNumAliveConnections();
    }

    if (secureServerCnxnFactory != null) {
        numAliveConnections += secureServerCnxnFactory.getNumAliveConnections();
    }

    return numAliveConnections;
}

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eolivelli
Copy link
Contributor

can you please merge with current master in order to pick up recent GH actions improvements ?

@Anonymitaet
Copy link
Member

@xiaotongwang1 thanks for your code work. For this PR, do we need to update docs?

@xiaotongwang1
Copy link
Contributor Author

@xiaotongwang1 thanks for your code work. For this PR, do we need to update docs?

No need to update.

@codelipenghui codelipenghui removed this from the 2.8.0 milestone May 21, 2021
@codelipenghui
Copy link
Contributor

@xiaotongwang1 We have removed ZooKeeperServerAspect.java by #10533, I will close this issue first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[improve] zookeeper connections monitor data
6 participants