ARTEMIS-1365 Advisory consumers listed in Console #1647

gaohoward · 2017-11-07T04:11:08Z

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.

andytaylor · 2017-11-07T08:23:27Z

Howard, This just turns off advisories completely from a management pov, we should have a switch to turn them off or on as well

andytaylor · 2017-11-07T08:32:49Z

Also you need to make sure that the JMX objects aren't created at all for both addresses and queues

michaelandrepearce · 2017-11-07T08:48:18Z

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. As such i agree with Andy having this shown or not should be toggle-able.

Also another note, but it seems AddressInfo should have a boolean field 'isInternal' to denote if it is an internal address, that can be set by the OpenWire protocol on creation of the address rather than building protocol logic into the server modules outside the protocol layers. Im discussing this section:

"public class AddressInfo {

//from openwire
public static final SimpleString ADVISORY_TOPIC = new SimpleString("ActiveMQ.Advisory.");
"

that was recently added in #1587

Also a query not related to this PR, but from my understanding advisories are very much like management notifications, yet i don't see it tapping into that feed to make the advisories?

michaelandrepearce · 2017-11-07T09:25:24Z

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);


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.

michaelandrepearce · 2017-11-07T09:26:28Z

artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/AddressInfo.java

+      return isInternal(this.name);
+   }
+
+   public static boolean isInternal(SimpleString address) {


Shouldn't be needed should be an attribute on the AddressInfo

gaohoward · 2017-11-09T03:07:24Z

@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).

michaelandrepearce · 2017-11-09T08:13:13Z

@gaohoward ill take the question part to dev mail probably should have done it in first place. And just concentrate on pr details here

gaohoward · 2017-11-09T09:35:03Z

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?

andytaylor · 2017-11-09T09:52:59Z

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 ***@***.***> 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 <#1647 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAt5svcGhiQjgyI_ryuUzJlDcVWlxz_Mks5s0sdIgaJpZM4QUPS5> .

gaohoward · 2017-11-09T11:02:25Z

ok, make sense to me.

michaelandrepearce · 2017-11-09T13:27:52Z

Still would like the logic to sit within openwire protocol to set an address to internal and not within core classes, addressinfo should just hold a Boolean flag of if it should be treated as internal address or not, this way if future cases the logic is held only in the specific protocol

clebertsuconic · 2017-11-09T23:04:47Z

@andytaylor / @mtaylor this is an area of your expertise.. I will let you guys merge this one.

gaohoward · 2017-11-10T07:17:38Z

@michaelandrepearce I'll find a way to remove the protocol specific from addressInfo.

gaohoward · 2017-11-13T11:34:47Z

I'll update this again. (Adding some doc about the newly added properties)
Pls hold the merge.

gaohoward · 2017-11-14T15:33:54Z

@mtaylor @michaelandrepearce guys I think it's ready for review now. Thanks!

andytaylor · 2017-11-15T09:54:16Z

do we need to move the AddressInfo class?

gaohoward · 2017-11-15T10:58:07Z

@andytaylor got a simple way to keep the AddressInfo. will upload soon after local test check.

gaohoward · 2017-11-15T11:14:57Z

@andytaylor I've made changes to keep AddressInfo in its original package. can you take a look at it?
Thanks.

andytaylor · 2017-11-15T14:56:17Z

looks ok to me

michaelandrepearce

I don’t see any update in the code to persist the extra field so it’s kept in restart.

Like wise I don’t see any way of configuring this in broker xml. (this one could be done later as separate pr)

Also a few left over system outs

michaelandrepearce · 2017-11-15T23:01:16Z

...ver/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);


Should be removed

michaelandrepearce · 2017-11-15T23:06:23Z

...col/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);


Should be removed

andytaylor · 2017-11-17T08:15:25Z

@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.

michaelandrepearce · 2017-11-17T08:44:16Z

@andytaylor can we remove the sysouts before merging.

andytaylor · 2017-11-17T08:52:14Z

@michaelandrepearce yes, I will wait before merging

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. Also refactors a code to remove Openwire specific contants from AddressInfo class.

gaohoward · 2017-11-17T09:45:44Z

Sorry about the System.out, I've remove them.
Thanks guys!

michaelandrepearce · 2017-11-17T10:04:45Z

@gaohoward thanks for all the changes, appreciate the effort, great work. +1 from me.

gaohoward · 2017-11-17T10:20:53Z

Thanks @michaelandrepearce, great help!

michaelandrepearce reviewed Nov 7, 2017

View reviewed changes

gaohoward force-pushed the amaster_1365 branch from 02585ea to 3041a84 Compare November 13, 2017 11:31

gaohoward force-pushed the amaster_1365 branch 2 times, most recently from bf5a323 to 6dc53af Compare November 14, 2017 04:44

gaohoward force-pushed the amaster_1365 branch from 6dc53af to d6c30eb Compare November 15, 2017 11:13

michaelandrepearce reviewed Nov 15, 2017

View reviewed changes

gaohoward force-pushed the amaster_1365 branch from d6c30eb to 5fe88e1 Compare November 17, 2017 09:44

michaelandrepearce approved these changes Nov 17, 2017

View reviewed changes

asfgit merged commit 5fe88e1 into apache:master Nov 20, 2017

asfgit pushed a commit that referenced this pull request Nov 20, 2017

This closes #1647 ARTEMIS-1365 Advisory consumers listed in Console

9a8055b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARTEMIS-1365 Advisory consumers listed in Console #1647

ARTEMIS-1365 Advisory consumers listed in Console #1647

gaohoward commented Nov 7, 2017

andytaylor commented Nov 7, 2017

andytaylor commented Nov 7, 2017

michaelandrepearce commented Nov 7, 2017 •

edited

michaelandrepearce Nov 7, 2017

michaelandrepearce Nov 7, 2017

gaohoward commented Nov 9, 2017

michaelandrepearce commented Nov 9, 2017 •

edited

gaohoward commented Nov 9, 2017

andytaylor commented Nov 9, 2017 via email

gaohoward commented Nov 9, 2017

michaelandrepearce commented Nov 9, 2017 •

edited

clebertsuconic commented Nov 9, 2017

gaohoward commented Nov 10, 2017

gaohoward commented Nov 13, 2017 •

edited

gaohoward commented Nov 14, 2017

andytaylor commented Nov 15, 2017

gaohoward commented Nov 15, 2017

gaohoward commented Nov 15, 2017

andytaylor commented Nov 15, 2017

michaelandrepearce left a comment •

edited

michaelandrepearce Nov 15, 2017

michaelandrepearce Nov 15, 2017

andytaylor commented Nov 17, 2017

michaelandrepearce commented Nov 17, 2017

andytaylor commented Nov 17, 2017

gaohoward commented Nov 17, 2017

michaelandrepearce commented Nov 17, 2017 •

edited

gaohoward commented Nov 17, 2017

ARTEMIS-1365 Advisory consumers listed in Console #1647

ARTEMIS-1365 Advisory consumers listed in Console #1647

Conversation

gaohoward commented Nov 7, 2017

andytaylor commented Nov 7, 2017

andytaylor commented Nov 7, 2017

michaelandrepearce commented Nov 7, 2017 • edited

michaelandrepearce Nov 7, 2017

Choose a reason for hiding this comment

michaelandrepearce Nov 7, 2017

Choose a reason for hiding this comment

gaohoward commented Nov 9, 2017

michaelandrepearce commented Nov 9, 2017 • edited

gaohoward commented Nov 9, 2017

andytaylor commented Nov 9, 2017 via email

gaohoward commented Nov 9, 2017

michaelandrepearce commented Nov 9, 2017 • edited

clebertsuconic commented Nov 9, 2017

gaohoward commented Nov 10, 2017

gaohoward commented Nov 13, 2017 • edited

gaohoward commented Nov 14, 2017

andytaylor commented Nov 15, 2017

gaohoward commented Nov 15, 2017

gaohoward commented Nov 15, 2017

andytaylor commented Nov 15, 2017

michaelandrepearce left a comment • edited

Choose a reason for hiding this comment

michaelandrepearce Nov 15, 2017

Choose a reason for hiding this comment

michaelandrepearce Nov 15, 2017

Choose a reason for hiding this comment

andytaylor commented Nov 17, 2017

michaelandrepearce commented Nov 17, 2017

andytaylor commented Nov 17, 2017

gaohoward commented Nov 17, 2017

michaelandrepearce commented Nov 17, 2017 • edited

gaohoward commented Nov 17, 2017

michaelandrepearce commented Nov 7, 2017 •

edited

michaelandrepearce commented Nov 9, 2017 •

edited

michaelandrepearce commented Nov 9, 2017 •

edited

gaohoward commented Nov 13, 2017 •

edited

michaelandrepearce left a comment •

edited

michaelandrepearce commented Nov 17, 2017 •

edited