You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/09/28 04:32:38 UTC
[GitHub] [pulsar] BewareMyPower opened a new pull request #12217: Disable stats recorder for built-in PulsarClient
BewareMyPower opened a new pull request #12217:
URL: https://github.com/apache/pulsar/pull/12217
### Motivation
In `PulsarService`, there's a built-in client used in many places, like topic compaction, system topics read/write. It could also be used in protocol handlers. Different with the manually created Pulsar client, it can read the configured authentication params and TLS related configs for broker-to-broker communication.
However, it doesn't change the default `statsIntervalSeconds` config, which means each time a producer/consumer/reader is created, a stats recorder (`ProducerStatsRecorderImpl` or `ConsumerStatsRecorderImpl`) will be created, in which there's a Netty `TimerTask` that prints stats periodically (the default interval is 1 minute). If there're many producers/consumers/readers created by this built-in client, the unnecessary CPU usage will be high.
### Modifications
Configure the `statsIntervalSeconds` with zero value to disable stats recorder.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change is a trivial rework / code cleanup without any test coverage.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] hangc0276 commented on a change in pull request #12217: Disable stats recorder for built-in PulsarClient
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on a change in pull request #12217:
URL: https://github.com/apache/pulsar/pull/12217#discussion_r717220264
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -1327,6 +1327,8 @@ public synchronized PulsarClient getClient() throws PulsarServerException {
this.getConfiguration().getBrokerClientAuthenticationPlugin(),
this.getConfiguration().getBrokerClientAuthenticationParameters()));
}
+
+ conf.setStatsIntervalSeconds(0);
Review comment:
For the pulsar replicator, it also call this `getClient` method to init pulsar client, which should retain the stats. So we'd better add a parameter to decide whether keep the client stats.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -1327,6 +1327,8 @@ public synchronized PulsarClient getClient() throws PulsarServerException {
this.getConfiguration().getBrokerClientAuthenticationPlugin(),
this.getConfiguration().getBrokerClientAuthenticationParameters()));
}
+
+ conf.setStatsIntervalSeconds(0);
Review comment:
Ok, it doesn't need the stats in replicator
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a change in pull request #12217: Disable stats recorder for built-in PulsarClient
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #12217:
URL: https://github.com/apache/pulsar/pull/12217#discussion_r717221392
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -1327,6 +1327,8 @@ public synchronized PulsarClient getClient() throws PulsarServerException {
this.getConfiguration().getBrokerClientAuthenticationPlugin(),
this.getConfiguration().getBrokerClientAuthenticationParameters()));
}
+
+ conf.setStatsIntervalSeconds(0);
Review comment:
Why do we need the stats in replicator?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower merged pull request #12217: Disable stats recorder for built-in PulsarClient
Posted by GitBox <gi...@apache.org>.
BewareMyPower merged pull request #12217:
URL: https://github.com/apache/pulsar/pull/12217
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #12217: Disable stats recorder for built-in PulsarClient
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #12217:
URL: https://github.com/apache/pulsar/pull/12217#issuecomment-929164628
My only concern is that some users conduct some business analysis by collecting this log. At least this change needs to be explained in the Release, right?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #12217: Disable stats recorder for built-in PulsarClient
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #12217:
URL: https://github.com/apache/pulsar/pull/12217#issuecomment-929175576
> My only concern is that some users conduct some business analysis by collecting this log. At least this change needs to be explained in the Release, right?
Yeah, you're right. Anyway it's a trade-off. And I'm curious about how can we explain it in release notes?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower merged pull request #12217: Disable stats recorder for built-in PulsarClient
Posted by GitBox <gi...@apache.org>.
BewareMyPower merged pull request #12217:
URL: https://github.com/apache/pulsar/pull/12217
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #12217: Disable stats recorder for built-in PulsarClient
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #12217:
URL: https://github.com/apache/pulsar/pull/12217#issuecomment-929175576
> My only concern is that some users conduct some business analysis by collecting this log. At least this change needs to be explained in the Release, right?
Yeah, you're right. Anyway it's a trade-off. And I'm curious about how can we explain it in release notes?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] hangc0276 commented on a change in pull request #12217: Disable stats recorder for built-in PulsarClient
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on a change in pull request #12217:
URL: https://github.com/apache/pulsar/pull/12217#discussion_r717239136
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -1327,6 +1327,8 @@ public synchronized PulsarClient getClient() throws PulsarServerException {
this.getConfiguration().getBrokerClientAuthenticationPlugin(),
this.getConfiguration().getBrokerClientAuthenticationParameters()));
}
+
+ conf.setStatsIntervalSeconds(0);
Review comment:
Ok, it doesn't need the stats in replicator
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #12217: Disable stats recorder for built-in PulsarClient
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #12217:
URL: https://github.com/apache/pulsar/pull/12217#issuecomment-929164628
My only concern is that some users conduct some business analysis by collecting this log. At least this change needs to be explained in the Release, right?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a change in pull request #12217: Disable stats recorder for built-in PulsarClient
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #12217:
URL: https://github.com/apache/pulsar/pull/12217#discussion_r717221392
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -1327,6 +1327,8 @@ public synchronized PulsarClient getClient() throws PulsarServerException {
this.getConfiguration().getBrokerClientAuthenticationPlugin(),
this.getConfiguration().getBrokerClientAuthenticationParameters()));
}
+
+ conf.setStatsIntervalSeconds(0);
Review comment:
Why do we need the stats in replicator?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] hangc0276 commented on a change in pull request #12217: Disable stats recorder for built-in PulsarClient
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on a change in pull request #12217:
URL: https://github.com/apache/pulsar/pull/12217#discussion_r717220264
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -1327,6 +1327,8 @@ public synchronized PulsarClient getClient() throws PulsarServerException {
this.getConfiguration().getBrokerClientAuthenticationPlugin(),
this.getConfiguration().getBrokerClientAuthenticationParameters()));
}
+
+ conf.setStatsIntervalSeconds(0);
Review comment:
For the pulsar replicator, it also call this `getClient` method to init pulsar client, which should retain the stats. So we'd better add a parameter to decide whether keep the client stats.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org