You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by hzbarcea <gi...@git.apache.org> on 2014/11/26 06:17:25 UTC
[GitHub] incubator-brooklyn pull request: [BROOKLYN-92] Configure brokerNam...
GitHub user hzbarcea opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/359
[BROOKLYN-92] Configure brokerName for ActiveMQ
Issue raised in jira: https://issues.apache.org/jira/browse/BROOKLYN-92
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/hzbarcea/incubator-brooklyn master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/359.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 #359
----
commit 3c6ba532bc49bcbc76ccd147ec107d2e866e2c69
Author: Hadrian Zbarcea <ha...@apache.org>
Date: 2014-11-26T05:10:40Z
[BROOKLYN-92] Configure brokerName for ActiveMQ
----
---
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-brooklyn pull request: [BROOKLYN-92] Configure brokerNam...
Posted by hzbarcea <gi...@git.apache.org>.
Github user hzbarcea commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/359#discussion_r21048169
--- Diff: software/messaging/src/main/java/brooklyn/entity/messaging/activemq/ActiveMQDestinationImpl.java ---
@@ -39,9 +42,11 @@ public ActiveMQDestinationImpl() {
public void onManagementStarting() {
super.onManagementStarting();
- //assume just one BrokerName at this endpoint
+ getBrokerName();
+ Preconditions.checkNotNull(brokerName, "ActiveMQ broker name must be specified");
--- End diff --
Ack, done.
---
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-brooklyn pull request: [BROOKLYN-92] Configure brokerNam...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/359#discussion_r21036276
--- Diff: software/messaging/src/main/java/brooklyn/entity/messaging/activemq/ActiveMQDestinationImpl.java ---
@@ -52,4 +57,12 @@ public void onManagementStarting() {
protected void disconnectSensors() {
if (jmxFeed != null) jmxFeed.stop();
}
+
+ protected String getBrokerName() {
+ if (brokerName == null) {
+ EntityLocal parent = (EntityLocal)getParent();
--- End diff --
You don't need to cast to an `EntityLocal` to call `getAttribute()` on `parent`.
---
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-brooklyn pull request: [BROOKLYN-92] Configure brokerNam...
Posted by hzbarcea <gi...@git.apache.org>.
Github user hzbarcea commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/359#issuecomment-64940140
Thanks for the thorough review. I think it's better now.
---
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-brooklyn pull request: [BROOKLYN-92] Configure brokerNam...
Posted by hzbarcea <gi...@git.apache.org>.
Github user hzbarcea commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/359#discussion_r21048124
--- Diff: software/messaging/src/main/java/brooklyn/entity/messaging/activemq/ActiveMQDestinationImpl.java ---
@@ -52,4 +57,12 @@ public void onManagementStarting() {
protected void disconnectSensors() {
if (jmxFeed != null) jmxFeed.stop();
}
+
+ protected String getBrokerName() {
+ if (brokerName == null) {
+ EntityLocal parent = (EntityLocal)getParent();
--- End diff --
Ack. Done
---
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-brooklyn pull request: [BROOKLYN-92] Configure brokerNam...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/359#discussion_r21036388
--- Diff: software/messaging/src/main/java/brooklyn/entity/messaging/activemq/ActiveMQQueueImpl.java ---
@@ -41,7 +41,7 @@ public String getQueueName() {
}
public void create() {
- if (log.isDebugEnabled()) log.debug("{} adding queue {} to broker {}", new Object[] {this, getName(), jmxHelper.getAttribute(brokerMBeanName, "BrokerId")});
--- End diff --
Let's keep the `isDebugEnabled` check in place.
---
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-brooklyn pull request: [BROOKLYN-92] Configure brokerNam...
Posted by hzbarcea <gi...@git.apache.org>.
Github user hzbarcea commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/359#discussion_r21048237
--- Diff: software/messaging/src/main/java/brooklyn/entity/messaging/activemq/ActiveMQQueueImpl.java ---
@@ -41,7 +41,7 @@ public String getQueueName() {
}
public void create() {
- if (log.isDebugEnabled()) log.debug("{} adding queue {} to broker {}", new Object[] {this, getName(), jmxHelper.getAttribute(brokerMBeanName, "BrokerId")});
--- End diff --
Uhm, let's not. This is extra check is uselss and non-idiomatic for slf4j. There are quite a bunch of other places in the code that use the same pattern that should be cleaned up.
---
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-brooklyn pull request: [BROOKLYN-92] Configure brokerNam...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/359#discussion_r21036204
--- Diff: software/messaging/src/main/java/brooklyn/entity/messaging/activemq/ActiveMQDestinationImpl.java ---
@@ -39,9 +42,11 @@ public ActiveMQDestinationImpl() {
public void onManagementStarting() {
super.onManagementStarting();
- //assume just one BrokerName at this endpoint
+ getBrokerName();
+ Preconditions.checkNotNull(brokerName, "ActiveMQ broker name must be specified");
--- End diff --
I think it would be better to replace all references to `brokerName` with a call to `getBrokerName()`.
---
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-brooklyn pull request: [BROOKLYN-92] Configure brokerNam...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/359#discussion_r21035548
--- Diff: software/messaging/src/main/java/brooklyn/entity/messaging/activemq/ActiveMQBroker.java ---
@@ -56,6 +56,10 @@
public static final BasicConfigKey<String> MIRROR_URL = new BasicConfigKey<String>(String.class, "activemq.install.mirror.url", "URL of mirror",
"http://www.mirrorservice.org/sites/ftp.apache.org/activemq");
+ @SetFromFlag("brokerName")
+ public static final AttributeSensorAndConfigKey<String,String> BROKER_NAME = new BasicAttributeSensorAndConfigKey<String>(
+ String.class, "activemq.brokerName", "ActiveMQ Broker Name", "localhost");
--- End diff --
Pretty trivial but you could also write this in a simpler form:
`ConfigKeys.newStringSensorAndConfigKey("activemq.brokerName", "ActiveMQ Broker Name", "localhost")`
---
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-brooklyn pull request: [BROOKLYN-92] Configure brokerNam...
Posted by hzbarcea <gi...@git.apache.org>.
Github user hzbarcea commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/359#discussion_r21048177
--- Diff: software/messaging/src/main/java/brooklyn/entity/messaging/activemq/ActiveMQDestinationImpl.java ---
@@ -52,4 +57,12 @@ public void onManagementStarting() {
protected void disconnectSensors() {
if (jmxFeed != null) jmxFeed.stop();
}
+
+ protected String getBrokerName() {
+ if (brokerName == null) {
+ EntityLocal parent = (EntityLocal)getParent();
+ brokerName = parent != null ? parent.getAttribute(ActiveMQBroker.BROKER_NAME) : null;
--- End diff --
It should not, but coding errors could lead to NPEs if the wiring is not done right. Refactored the code to simplify as you suggest but added another check that would throw a more meaninful message than an NPE>
---
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-brooklyn pull request: [BROOKLYN-92] Configure brokerNam...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/359#issuecomment-65085226
Looks good. Merging :frog:
---
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-brooklyn pull request: [BROOKLYN-92] Configure brokerNam...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/359#issuecomment-64905670
@hzbarcea Thanks for the contribution, doubly so since it's tested! I've made a few comments that shouldn't take too long to address then this is good to merge.
---
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-brooklyn pull request: [BROOKLYN-92] Configure brokerNam...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/359#discussion_r21036533
--- Diff: software/messaging/src/test/java/brooklyn/entity/messaging/activemq/ActiveMQIntegrationTest.java ---
@@ -80,7 +80,7 @@ public void canStartupAndShutdown() throws Exception {
activeMQ = app.createAndManageChild(EntitySpec.create(ActiveMQBroker.class));
activeMQ.start(ImmutableList.of(testLocation));
- EntityTestUtils.assertAttributeEqualsEventually(ImmutableMap.of("timeout", 10*60*1000), activeMQ, Startable.SERVICE_UP, true);
+ EntityTestUtils.assertAttributeEqualsEventually(ImmutableMap.of("timeout", 60*1000), activeMQ, Startable.SERVICE_UP, true);
--- End diff --
I believe we have these really long time outs because integration tests often take much much longer than you would expect when running on low powered CI machines (especially when it has low bandwidth and has to download and install the program). Although they can be annoying when running locally I think it's best we keep them in place.
---
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-brooklyn pull request: [BROOKLYN-92] Configure brokerNam...
Posted by hzbarcea <gi...@git.apache.org>.
Github user hzbarcea commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/359#discussion_r21048252
--- Diff: software/messaging/src/test/java/brooklyn/entity/messaging/activemq/ActiveMQIntegrationTest.java ---
@@ -80,7 +80,7 @@ public void canStartupAndShutdown() throws Exception {
activeMQ = app.createAndManageChild(EntitySpec.create(ActiveMQBroker.class));
activeMQ.start(ImmutableList.of(testLocation));
- EntityTestUtils.assertAttributeEqualsEventually(ImmutableMap.of("timeout", 10*60*1000), activeMQ, Startable.SERVICE_UP, true);
+ EntityTestUtils.assertAttributeEqualsEventually(ImmutableMap.of("timeout", 60*1000), activeMQ, Startable.SERVICE_UP, true);
--- End diff --
I am torn on this one. I understand your point, yet I've been bit many times by such long timeout. If something goes wrong it wastes a lot of your time. However this is unrelated to the enhancement this patch proposes. If we ever wanted to talk about the merits of timeouts it can be done at a later time. Change reverted per your suggestion.
---
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-brooklyn pull request: [BROOKLYN-92] Configure brokerNam...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/359#discussion_r21097112
--- Diff: software/messaging/src/main/java/brooklyn/entity/messaging/activemq/ActiveMQQueueImpl.java ---
@@ -41,7 +41,7 @@ public String getQueueName() {
}
public void create() {
- if (log.isDebugEnabled()) log.debug("{} adding queue {} to broker {}", new Object[] {this, getName(), jmxHelper.getAttribute(brokerMBeanName, "BrokerId")});
--- End diff --
Interesting. Thanks for the heads-up, we should clean this up. Is there a good reference for idiomatic SLF4J usage?
---
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-brooklyn pull request: [BROOKLYN-92] Configure brokerNam...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/359#discussion_r21036362
--- Diff: software/messaging/src/main/java/brooklyn/entity/messaging/activemq/ActiveMQDestinationImpl.java ---
@@ -52,4 +57,12 @@ public void onManagementStarting() {
protected void disconnectSensors() {
if (jmxFeed != null) jmxFeed.stop();
}
+
+ protected String getBrokerName() {
+ if (brokerName == null) {
+ EntityLocal parent = (EntityLocal)getParent();
+ brokerName = parent != null ? parent.getAttribute(ActiveMQBroker.BROKER_NAME) : null;
--- End diff --
Will `parent` ever be null? I think it's an error if an `ActiveMQDestination` has no `ActiveMQBroker` parent. So perhaps this whole method could be replaced with a simple `return getParent().getAttribute(ActiveMQBroker.BROKER_NAME)`.
---
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-brooklyn pull request: [BROOKLYN-92] Configure brokerNam...
Posted by hzbarcea <gi...@git.apache.org>.
Github user hzbarcea commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/359#discussion_r21048083
--- Diff: software/messaging/src/main/java/brooklyn/entity/messaging/activemq/ActiveMQBroker.java ---
@@ -56,6 +56,10 @@
public static final BasicConfigKey<String> MIRROR_URL = new BasicConfigKey<String>(String.class, "activemq.install.mirror.url", "URL of mirror",
"http://www.mirrorservice.org/sites/ftp.apache.org/activemq");
+ @SetFromFlag("brokerName")
+ public static final AttributeSensorAndConfigKey<String,String> BROKER_NAME = new BasicAttributeSensorAndConfigKey<String>(
+ String.class, "activemq.brokerName", "ActiveMQ Broker Name", "localhost");
--- End diff --
Thanks for tip, will do.
---
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-brooklyn pull request: [BROOKLYN-92] Configure brokerNam...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/359
---
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.
---