[GitHub] activemq-artemis pull request #1647: ARTEMIS-1365 Advisory consumers listed ...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
32 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1647: ARTEMIS-1365 Advisory consumers listed ...

michaelandrepearce-2
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 <[hidden email]>
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.

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...

michaelandrepearce-2
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


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
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


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
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)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1647: ARTEMIS-1365 Advisory consumers listed ...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1647: ARTEMIS-1365 Advisory consumers listed ...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
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


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
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).


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
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


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
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?



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
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 <[hidden email]> 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>
    > .
    >



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user gaohoward commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1647
 
    ok, make sense to me.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
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)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
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!


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
Github user andytaylor commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1647
 
    do we need to move the AddressInfo class?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1647: ARTEMIS-1365 Advisory consumers listed in Cons...

michaelandrepearce-2
In reply to this post by michaelandrepearce-2
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.


---
12