You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pulsar.apache.org by merlimat <gi...@git.apache.org> on 2017/06/23 00:18:44 UTC
[GitHub] incubator-pulsar pull request #521: Fixed problem in starting ZK server for ...
GitHub user merlimat opened a pull request:
https://github.com/apache/incubator-pulsar/pull/521
Fixed problem in starting ZK server for standalone mode
### Motivation
In 1.18 release there is a problem in starting the standalone broker.
```
$ bin/pulsar standalone
2017-06-22 17:15:11,865 - INFO - [main:LocalBookkeeperEnsemble@100] - Running 1 bookie(s).
2017-06-22 17:15:11,893 - INFO - [main:LocalBookkeeperEnsemble@124] - Starting ZK server
2017-06-22 17:15:11,960 - ERROR - [main:LocalBookkeeperEnsemble@142] - Exception while instantiating ZooKeeper
java.lang.IllegalArgumentException: Collector already registered that provides name: zookeeper_server_znode_count
at io.prometheus.client.CollectorRegistry.register(CollectorRegistry.java:54)
at io.prometheus.client.Collector.register(Collector.java:128)
at io.prometheus.client.Collector.register(Collector.java:121)
at com.yahoo.pulsar.zookeeper.ZooKeeperServerAspect.timedProcessRequest(ZooKeeperServerAspect.java:46)
at org.apache.zookeeper.server.ZooKeeperServer.<init>(ZooKeeperServer.java:191)
at org.apache.zookeeper.server.ZooKeeperServer.<init>(ZooKeeperServer.java:224)
at com.yahoo.pulsar.zookeeper.LocalBookkeeperEnsemble.runZookeeper(LocalBookkeeperEnsemble.java:136)
at com.yahoo.pulsar.zookeeper.LocalBookkeeperEnsemble.start(LocalBookkeeperEnsemble.java:228)
at com.yahoo.pulsar.PulsarStandaloneStarter.start(PulsarStandaloneStarter.java:149)
at com.yahoo.pulsar.PulsarStandaloneStarter.main(PulsarStandaloneStarter.java:197)
2017-06-22 17:15:11,969 - INFO - [main:LocalBookkeeperEnsemble@300] - server 127.0.0.1:2181 not up java.net.ConnectException: Connection refused (Connection refused)
2017-06-22 17:15:12,220 - INFO - [main:LocalBookkeeperEnsemble@300] - server 127.0.0.1:2181 not up java.net.ConnectException: Connection refused (Connection refused)
2017-06-22 17:15:12,471 - INFO - [main:LocalBookkeeperEnsemble@300] - server 127.0.0.1:2181 not up java.net.ConnectException: Connection refused (Connection refused)
2017-06-22 17:15:12,727 - INFO - [main:LocalBookkeeperEnsemble@300] - server 127.0.0.1:2181 not up java.net.ConnectException: Connection refused (Connection refused)
```
For some reason this problem doesn't happen when compiling from source, but just on the binary distribution.
### Modifications
Ensure metrics are registered only once in the process.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/merlimat/pulsar master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-pulsar/pull/521.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 #521
----
commit db61ba35d371d42d5a351d84146aeae32104ced9
Author: Matteo Merli <mm...@apache.org>
Date: 2017-06-23T00:12:46Z
Fixed problem in starting ZK server for standalone mode
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-pulsar pull request #521: Fixed problem in starting ZK server for ...
Posted by rdhabalia <gi...@git.apache.org>.
Github user rdhabalia commented on a diff in the pull request:
https://github.com/apache/incubator-pulsar/pull/521#discussion_r123651843
--- Diff: pulsar-zookeeper/src/main/java/com/yahoo/pulsar/zookeeper/ZooKeeperServerAspect.java ---
@@ -31,15 +31,26 @@
*/
@Aspect
public class ZooKeeperServerAspect {
+ private static boolean metricsRegistered = false;
+
@Pointcut("execution(org.apache.zookeeper.server.ZooKeeperServer.new(..))")
- public void processRequest() {
+ public void zkServerConstructorPointCut() {
}
- @After("processRequest()")
- public void timedProcessRequest(JoinPoint joinPoint) throws Throwable {
+ @After("zkServerConstructorPointCut()")
+ public void zkServerConstructor(JoinPoint joinPoint) throws Throwable {
// ZooKeeperServer instance was created
ZooKeeperServer zkServer = (ZooKeeperServer) joinPoint.getThis();
+ synchronized (ZooKeeperServerAspect.class) {
+ if (metricsRegistered) {
--- End diff --
> it's not double checked, since the mutex it's upfront :)
Oops. yes, my bad.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-pulsar pull request #521: Fixed problem in starting ZK server for ...
Posted by merlimat <gi...@git.apache.org>.
Github user merlimat closed the pull request at:
https://github.com/apache/incubator-pulsar/pull/521
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-pulsar pull request #521: Fixed problem in starting ZK server for ...
Posted by rdhabalia <gi...@git.apache.org>.
Github user rdhabalia commented on a diff in the pull request:
https://github.com/apache/incubator-pulsar/pull/521#discussion_r123650995
--- Diff: pulsar-zookeeper/src/main/java/com/yahoo/pulsar/zookeeper/ZooKeeperServerAspect.java ---
@@ -31,15 +31,26 @@
*/
@Aspect
public class ZooKeeperServerAspect {
+ private static boolean metricsRegistered = false;
+
@Pointcut("execution(org.apache.zookeeper.server.ZooKeeperServer.new(..))")
- public void processRequest() {
+ public void zkServerConstructorPointCut() {
}
- @After("processRequest()")
- public void timedProcessRequest(JoinPoint joinPoint) throws Throwable {
+ @After("zkServerConstructorPointCut()")
+ public void zkServerConstructor(JoinPoint joinPoint) throws Throwable {
// ZooKeeperServer instance was created
ZooKeeperServer zkServer = (ZooKeeperServer) joinPoint.getThis();
+ synchronized (ZooKeeperServerAspect.class) {
+ if (metricsRegistered) {
--- End diff --
I think it's a double-checked locking and may not be still thread-safe. should we make `metricsRegistered` volatile?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-pulsar pull request #521: Fixed problem in starting ZK server for ...
Posted by merlimat <gi...@git.apache.org>.
Github user merlimat commented on a diff in the pull request:
https://github.com/apache/incubator-pulsar/pull/521#discussion_r123651694
--- Diff: pulsar-zookeeper/src/main/java/com/yahoo/pulsar/zookeeper/ZooKeeperServerAspect.java ---
@@ -31,15 +31,26 @@
*/
@Aspect
public class ZooKeeperServerAspect {
+ private static boolean metricsRegistered = false;
+
@Pointcut("execution(org.apache.zookeeper.server.ZooKeeperServer.new(..))")
- public void processRequest() {
+ public void zkServerConstructorPointCut() {
}
- @After("processRequest()")
- public void timedProcessRequest(JoinPoint joinPoint) throws Throwable {
+ @After("zkServerConstructorPointCut()")
+ public void zkServerConstructor(JoinPoint joinPoint) throws Throwable {
// ZooKeeperServer instance was created
ZooKeeperServer zkServer = (ZooKeeperServer) joinPoint.getThis();
+ synchronized (ZooKeeperServerAspect.class) {
+ if (metricsRegistered) {
--- End diff --
so, the variable is only read/written within the mutex and there is no need to use volatile, because the mutex implies here a full memory barrier, so all variables needs to be committed to memory and the execution cannot be reordered
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-pulsar issue #521: Fixed problem in starting ZK server for standal...
Posted by merlimat <gi...@git.apache.org>.
Github user merlimat commented on the issue:
https://github.com/apache/incubator-pulsar/pull/521
I think if we ever need to do a 1.18.1, this fix should be included.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-pulsar pull request #521: Fixed problem in starting ZK server for ...
Posted by merlimat <gi...@git.apache.org>.
Github user merlimat commented on a diff in the pull request:
https://github.com/apache/incubator-pulsar/pull/521#discussion_r123651279
--- Diff: pulsar-zookeeper/src/main/java/com/yahoo/pulsar/zookeeper/ZooKeeperServerAspect.java ---
@@ -31,15 +31,26 @@
*/
@Aspect
public class ZooKeeperServerAspect {
+ private static boolean metricsRegistered = false;
+
@Pointcut("execution(org.apache.zookeeper.server.ZooKeeperServer.new(..))")
- public void processRequest() {
+ public void zkServerConstructorPointCut() {
}
- @After("processRequest()")
- public void timedProcessRequest(JoinPoint joinPoint) throws Throwable {
+ @After("zkServerConstructorPointCut()")
+ public void zkServerConstructor(JoinPoint joinPoint) throws Throwable {
// ZooKeeperServer instance was created
ZooKeeperServer zkServer = (ZooKeeperServer) joinPoint.getThis();
+ synchronized (ZooKeeperServerAspect.class) {
+ if (metricsRegistered) {
--- End diff --
it's not double checked, since the mutex it's upfront :)
double checked would be trying to avoid the mutex in the best case, like:
```java
if (metricsRegistered) {
return;
} else {
synchronized (ZooKeeperServerAspect.class) {
if (metricsRegistered) {
return;
}
}
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---