You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by gaohoward <gi...@git.apache.org> on 2017/11/07 04:11:13 UTC
[GitHub] activemq-artemis pull request #1647: ARTEMIS-1365 Advisory consumers listed ...
GitHub user gaohoward opened a pull request:
https://github.com/apache/activemq-artemis/pull/1647
ARTEMIS-1365 Advisory consumers listed in Console
Openwire clients create consumers to advisory topics to receive
notifications. As a result there are internal queues created
on advisory topics. Those consumer shouldn't be exposed via
management APIs which are used by the Console
To fix that the broker doesn't register any queues from
advisory addresses.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gaohoward/activemq-artemis amaster_1365
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1647.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 #1647
----
commit 02585ea84d5a8719c56e5cf36b6d59aa33f2a3de
Author: Howard Gao <ho...@gmail.com>
Date: 2017-11-07T04:09:08Z
ARTEMIS-1365 Advisory consumers listed in Console
Openwire clients create consumers to advisory topics to receive
notifications. As a result there are internal queues created
on advisory topics. Those consumer shouldn't be exposed via
management APIs which are used by the Console
To fix that the broker doesn't register any queues from
advisory addresses.
----
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
@andytaylor I've made changes to keep AddressInfo in its original package. can you take a look at it?
Thanks.
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by andytaylor <gi...@git.apache.org>.
Github user andytaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
Howard, This just turns off advisories completely from a management pov, we should have a switch to turn them off or on as well
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by andytaylor <gi...@git.apache.org>.
Github user andytaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
Also you need to make sure that the JMX objects aren't created at all for both addresses and queues
---
[GitHub] activemq-artemis pull request #1647: ARTEMIS-1365 Advisory consumers listed ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1647
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
@andytaylor got a simple way to keep the AddressInfo. will upload soon after local test check.
---
[GitHub] activemq-artemis pull request #1647: ARTEMIS-1365 Advisory consumers listed ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1647#discussion_r151645918
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/openwire/management/OpenWireManagementTest.java ---
@@ -55,6 +87,19 @@ public void setUp() throws Exception {
@Override
protected void extraServerConfig(Configuration serverConfig) {
serverConfig.setJMXManagementEnabled(true);
+ if (useDefault) {
+ //don't set parameters explicitly
+ return;
+ }
+ Set<TransportConfiguration> acceptorConfigs = serverConfig.getAcceptorConfigurations();
+ for (TransportConfiguration tconfig : acceptorConfigs) {
+ if ("netty".equals(tconfig.getName())) {
+ Map<String, Object> params = tconfig.getExtraParams();
+ params.put("supportAdvisory", supportAdvisory);
+ params.put("suppressInternalManagementObjects", suppressJmx);
+ System.out.println("Now use properties: " + params);
--- End diff --
(sorry to be a pain)
---
[GitHub] activemq-artemis pull request #1647: ARTEMIS-1365 Advisory consumers listed ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1647#discussion_r151645844
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/openwire/management/OpenWireManagementTest.java ---
@@ -55,6 +87,19 @@ public void setUp() throws Exception {
@Override
protected void extraServerConfig(Configuration serverConfig) {
serverConfig.setJMXManagementEnabled(true);
+ if (useDefault) {
+ //don't set parameters explicitly
+ return;
+ }
+ Set<TransportConfiguration> acceptorConfigs = serverConfig.getAcceptorConfigurations();
+ for (TransportConfiguration tconfig : acceptorConfigs) {
+ if ("netty".equals(tconfig.getName())) {
+ Map<String, Object> params = tconfig.getExtraParams();
+ params.put("supportAdvisory", supportAdvisory);
+ params.put("suppressInternalManagementObjects", suppressJmx);
+ System.out.println("Now use properties: " + params);
--- End diff --
@gaohoward just spotted another
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by andytaylor <gi...@git.apache.org>.
Github user andytaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
@michaelandrepearce it is configured in the broker.xml file via the acceptor url so Im fine with that, dont think we need to persist it. Im happy to merge this.
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
@gaohoward thanks for all the changes, appreciate the effort. +1 from me.
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
@andytaylor can we remove the sysouts before merging.
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
Like wise if for some silly reason they attach with a durable we would need to be able to monitor queue depths (jmx)
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
I'll update this again. (Adding some doc about the newly added properties)
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
@mtaylor @michaelandrepearce guys I think it's ready for review now. Thanks!
---
[GitHub] activemq-artemis pull request #1647: ARTEMIS-1365 Advisory consumers listed ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1647#discussion_r149310333
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/AddressInfo.java ---
@@ -130,6 +130,10 @@ public String toString() {
}
public boolean isInternal() {
- return this.name.startsWith(ADVISORY_TOPIC);
+ return isInternal(this.name);
+ }
+
+ public static boolean isInternal(SimpleString address) {
--- End diff --
Shouldn't be needed should be an attribute on the AddressInfo
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by andytaylor <gi...@git.apache.org>.
Github user andytaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
looks ok to me
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
@andytaylor @michaelandrepearce guys agreed please don't merge, I'm making more changes now.
@michaelandrepearce I'm not sure about your query can you rephrase?
Advisories is openwire notification feature. We don't support the full feature, but we do have some implemented (as a by-product required for some other features in amq5 I don't remember exactly thou).
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
@michaelandrepearce I'll find a way to remove the protocol specific from addressInfo.
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
As a user if a remote client attaches to advisory or management addresses we would like to still see those so if something goes rogue we know what is attached.
---
[GitHub] activemq-artemis pull request #1647: ARTEMIS-1365 Advisory consumers listed ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1647#discussion_r151278990
--- Diff: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java ---
@@ -1221,6 +1240,7 @@ public Response processWireFormat(WireFormatInfo command) throws Exception {
public Response processAddDestination(DestinationInfo dest) throws Exception {
Response resp = null;
try {
+ System.out.println("___adding dest: " + dest);
--- End diff --
Should be removed
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by andytaylor <gi...@git.apache.org>.
Github user andytaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
do we need to move the AddressInfo class?
---
[GitHub] activemq-artemis pull request #1647: ARTEMIS-1365 Advisory consumers listed ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1647#discussion_r149310048
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/AddressInfo.java ---
@@ -130,6 +130,10 @@ public String toString() {
}
public boolean isInternal() {
- return this.name.startsWith(ADVISORY_TOPIC);
+ return isInternal(this.name);
--- End diff --
This should be a boolean field internal, that is set by the protocol layer, we shouldn't have protocol specific bits in the core models.
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
Thanks @michaelandrepearce, great help!
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
@andytaylor / @mtaylor this is an area of your expertise.. I will let you guys merge this one.
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
Sorry about the System.out, I've remove them.
Thanks guys!
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
Guys, so I added a parameter to OpenwireProtocolManager, named supportAdvisory. If set to false, no advisory addresses/queues will be created. Default is true.
However I'm not sure about the Console. When supportAdvisory is true, I would still want to hide the JMX objects (not register them but stored them in a list) so that the Console user won't be bothered by them. I think there should be a way to let the user to view those objects, may be adding one operation to the management api? I'm not sure, what you guys think of it?
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
ok, make sense to me.
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
Still would like the logic to sit within openwire protocol not within core classes, addressinfo should just hold a Boolean flag of if it should be treated as internal address or not.
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by andytaylor <gi...@git.apache.org>.
Github user andytaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
@michaelandrepearce yes, I will wait before merging
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by andytaylor <gi...@git.apache.org>.
Github user andytaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
As long as the JMX objects aren't created they won't appear on the console,
I would have a flag to disable them completely and one to hide them by not
creating JMX objects
On 9 November 2017 at 09:35, Howard Gao <no...@github.com> wrote:
> Guys, so I added a parameter to OpenwireProtocolManager, named
> supportAdvisory. If set to false, no advisory addresses/queues will be
> created. Default is true.
> However I'm not sure about the Console. When supportAdvisory is true, I
> would still want to hide the JMX objects (not register them but stored them
> in a list) so that the Console user won't be bothered by them. I think
> there should be a way to let the user to view those objects, may be adding
> one operation to the management api? I'm not sure, what you guys think of
> it?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/activemq-artemis/pull/1647#issuecomment-343098770>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAt5svcGhiQjgyI_ryuUzJlDcVWlxz_Mks5s0sdIgaJpZM4QUPS5>
> .
>
---
[GitHub] activemq-artemis pull request #1647: ARTEMIS-1365 Advisory consumers listed ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1647#discussion_r151277985
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/SimpleAddressManager.java ---
@@ -316,6 +316,7 @@ private void validateRoutingTypes(SimpleString addressName, Collection<RoutingTy
if (binding instanceof QueueBinding) {
final QueueBinding queueBinding = (QueueBinding) binding;
final RoutingType routingType = queueBinding.getQueue().getRoutingType();
+ System.out.println("got a queue binding........... " + queueBinding.getQueue().getName() + " rt: " + routingType);
--- End diff --
Should be removed
---
[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1647
@gaohoward ill take the question part to dev mail probabky should have done it in first place. And just concentrate on pr details here
---