[GitHub] activemq-artemis pull request #2401: ARTEMIS-1710 Allow management msgs to p...

classic Classic list List threaded Threaded
45 messages Options
123
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2401: ARTEMIS-1710 Allow management msgs to p...

jbertram-2
GitHub user franz1981 opened a pull request:

    https://github.com/apache/activemq-artemis/pull/2401

    ARTEMIS-1710 Allow management msgs to pass the global-max-size limit

   

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/franz1981/activemq-artemis ARTEMIS-1710

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/activemq-artemis/pull/2401.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 #2401
   
----
commit 69ff50157b32d84d71258a384ca4afd23ccc65f9
Author: Francesco Nigro <nigro.fra@...>
Date:   2018-10-25T16:26:17Z

    ARTEMIS-1710 Allow management msgs to pass the global-max-size limit

----


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

[GitHub] activemq-artemis pull request #2401: ARTEMIS-1710 Allow management msgs to p...

jbertram-2
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2401#discussion_r229013120
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/PagingManagerImpl.java ---
    @@ -437,12 +449,27 @@ public void processReload() throws Exception {
           }
        }
     
    +   private boolean ignoreGlobalMaxSize(SimpleString address) {
    +      if (this.addressPrefixesIgnoringGlobalMaxSize == null) {
    +         return false;
    +      } else {
    +         //for a small number of prefixes to check we can use just a linear search too :)
    +         for (SimpleString prefixes : this.addressPrefixesIgnoringGlobalMaxSize) {
    --- End diff --
   
    ATM it will have an array of just 1 element


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

[GitHub] activemq-artemis pull request #2401: ARTEMIS-1710 Allow management msgs to p...

jbertram-2
In reply to this post by jbertram-2
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2401#discussion_r229013748
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/PagingStoreImpl.java ---
    @@ -700,7 +720,7 @@ public boolean checkMemory(final Runnable runWhenAvailable) {
     
        @Override
        public void addSize(final int size) {
    -      boolean globalFull = pagingManager.addSize(size).isGlobalFull();
    +      boolean globalFull = !ignoreGlobalMaxSize && pagingManager.addSize(size).isGlobalFull();
    --- End diff --
   
    @clebertsuconic @mtaylor
    I'm not adding anything to the pagingManager to not trigger the other addresses policies: it makes sense?


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

[GitHub] activemq-artemis issue #2401: ARTEMIS-1710 Allow management msgs to pass the...

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

    https://github.com/apache/activemq-artemis/pull/2401
 
    @franz1981 this seems a bit dangerous, as a user/operator i set global max as a safety net even for management messages. Surely this would be better handled with simply setting better values per addresses.
   
    Or some other way to handle such needs of sorting the broker out like using JMX and Management Console.


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

[GitHub] activemq-artemis pull request #2401: ARTEMIS-1710 Allow management msgs to p...

jbertram-2
In reply to this post by jbertram-2
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2401#discussion_r229088266
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/PagingStoreImpl.java ---
    @@ -700,7 +720,7 @@ public boolean checkMemory(final Runnable runWhenAvailable) {
     
        @Override
        public void addSize(final int size) {
    -      boolean globalFull = pagingManager.addSize(size).isGlobalFull();
    +      boolean globalFull = !ignoreGlobalMaxSize && pagingManager.addSize(size).isGlobalFull();
    --- End diff --
   
    This would affect the reported size though, which for someone operating the broker is important it returns a correct and valid value.


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

[GitHub] activemq-artemis pull request #2401: ARTEMIS-1710 Allow management msgs to p...

jbertram-2
In reply to this post by jbertram-2
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2401#discussion_r229089174
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/PagingManagerImpl.java ---
    @@ -437,12 +449,27 @@ public void processReload() throws Exception {
           }
        }
     
    +   private boolean ignoreGlobalMaxSize(SimpleString address) {
    +      if (this.addressPrefixesIgnoringGlobalMaxSize == null) {
    +         return false;
    +      } else {
    +         //for a small number of prefixes to check we can use just a linear search too :)
    +         for (SimpleString prefixes : this.addressPrefixesIgnoringGlobalMaxSize) {
    --- End diff --
   
    why even code this to be more than one, concern here would be what else? could end up in non-managed space? Its a dangerous thing to have anything unbounded entirely. See my comment about possible alternative and safer approach with implementing a two tier global max sizing, one global  (all addresses) and one non-management max which would be set lower to allow head room for the management ones no matter what (all addresses except management)


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

[GitHub] activemq-artemis pull request #2401: ARTEMIS-1710 Allow management msgs to p...

jbertram-2
In reply to this post by jbertram-2
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2401#discussion_r229110673
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/PagingStoreImpl.java ---
    @@ -700,7 +720,7 @@ public boolean checkMemory(final Runnable runWhenAvailable) {
     
        @Override
        public void addSize(final int size) {
    -      boolean globalFull = pagingManager.addSize(size).isGlobalFull();
    +      boolean globalFull = !ignoreGlobalMaxSize && pagingManager.addSize(size).isGlobalFull();
    --- End diff --
   
    I agree: indeed it was one of my concern on it.
    On one side to not impact other addresses and trigger paging due to management messages and on the other side to maintain consistency of the metrics related to address utilization.
    Obviously just one of the paths can be chosen: I take your point and I agree to maintain correct metrics instead :+1:


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

[GitHub] activemq-artemis pull request #2401: ARTEMIS-1710 Allow management msgs to p...

jbertram-2
In reply to this post by jbertram-2
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2401#discussion_r229111410
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/PagingManagerImpl.java ---
    @@ -437,12 +449,27 @@ public void processReload() throws Exception {
           }
        }
     
    +   private boolean ignoreGlobalMaxSize(SimpleString address) {
    +      if (this.addressPrefixesIgnoringGlobalMaxSize == null) {
    +         return false;
    +      } else {
    +         //for a small number of prefixes to check we can use just a linear search too :)
    +         for (SimpleString prefixes : this.addressPrefixesIgnoringGlobalMaxSize) {
    --- End diff --
   
    I have implemented this feature lefting it opened to be extended to allow other address prefixes to be added too, but I can implementing it too just considering this specific use case too: just magement addresses messages.


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

[GitHub] activemq-artemis issue #2401: ARTEMIS-1710 Allow management msgs to pass the...

jbertram-2
In reply to this post by jbertram-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2401
 
    > Surely this would be better handled with simply setting better values per address.
   
    I could be wrong (and @mtaylor or @clebertsuconic can help me to understand the conditions I have found in the code), but it seems to me that the global-max-size can never be surpassed, regardless any configuration you'll set for a maching address.
    @lulf in the referenced issue has explained it very clearly: if you desire to have a global-max-size to be shared between all the addresses, but you need management messages to not being influenced by it, it doesn't seem possible with the current broker configuration.
    By default this option is disabled ie management addresses behave like we are always used to see.
   
    > Or at least implementing a two tier max, keeping the existing global-max-size as behaviour today (so users dont get a change in expectation), but implementing a secondary value thats global-non-management-max-size that could be a catch all at a level below.
   
    That's a good point: I like the idea of a `global-non-management-max-size` (while leaving `global-max-size` unbounded): what makes me not 100% sure of it is that to satisfy that same requirement it will introduce  a whole new feature, probably bigger. Let me think about it and hear the others opinions on it :+1:


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

[GitHub] activemq-artemis issue #2401: ARTEMIS-1710 Allow management msgs to pass the...

jbertram-2
In reply to this post by jbertram-2
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2401
 
    @franz1981 did you run a full testsuite on this?


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

[GitHub] activemq-artemis issue #2401: ARTEMIS-1710 Allow management msgs to pass the...

jbertram-2
In reply to this post by jbertram-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2401
 
    @clebertsuconic Let me run it while I'm working on an alternative version too: we can talk later about it mate?


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

Re: [GitHub] activemq-artemis issue #2401: ARTEMIS-1710 Allow management msgs to pass the...

CodeTechGuy
That is a sagaciously composed article and this is actually what I was
searching for. Great motivational counsel, and  more info
<http://www.roborace.co.uk/roborace-first-race-2019-lucas-di-grassi/>   just
as exactly what I require. I altogether making the most of your top to
bottom audit and have found the answer i was looking for.



--
Sent from: http://activemq.2283324.n4.nabble.com/ActiveMQ-Dev-f2368404.html
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2401: ARTEMIS-1710 Allow management msgs to p...

jbertram-2
In reply to this post by jbertram-2
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2401#discussion_r229640945
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/PagingManagerImpl.java ---
    @@ -437,12 +449,27 @@ public void processReload() throws Exception {
           }
        }
     
    +   private boolean ignoreGlobalMaxSize(SimpleString address) {
    +      if (this.addressPrefixesIgnoringGlobalMaxSize == null) {
    +         return false;
    +      } else {
    +         //for a small number of prefixes to check we can use just a linear search too :)
    +         for (SimpleString prefixes : this.addressPrefixesIgnoringGlobalMaxSize) {
    --- End diff --
   
    I taken the point and allowed that address Set to be just the one I need (if any), lefting the code be simple as it needs to be.


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

[GitHub] activemq-artemis issue #2401: ARTEMIS-1710 Allow management msgs to pass the...

jbertram-2
In reply to this post by jbertram-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2401
 
    @michaelandrepearce I've started working on the alternative one based on a second level global limit, but TBH I've already found quite complex to add such condition without breaking any of the current behaviours.
    I'm evaluating if it is really the case to add a more precise feature whose real use case would be the same of this one while involving a similar level of dangerousness.
    To obtain something similar to:
     ```xml
    <global-max-size>Any value or left it -1 to be auto-tuned</global-max-size>
    <management-address-ignore-global-max-size>true</management-address-ignore-global-max-size>
    ```
    the alternative solution will need to configure:
     ```xml
    <global-non-management-max-size>The original value used for global-max-size</global-non-management-max-size>
    <global-max-size>A value that is assumed to be unlimited</global-max-size>
    ```
     Only this configuration can give the 100% guarantee to the user that any management message won't be paged/fail/block regardless the current size of the non-management addresses.
    This type of configuration is dangerous for the same reasons this same PR is.
    In addition it will need to add:
   
    - a new value that identify an unlimited value, because -1 for `global-max-size` will force it to be auto-tuned
    - a way to auto-tune the `global-non-management-max-size` too (as we're doing it for `global-max-size`)
    - an impact on any code paths of the check that we perform now


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

[GitHub] activemq-artemis issue #2401: ARTEMIS-1710 Allow management msgs to pass the...

jbertram-2
In reply to this post by jbertram-2
Github user lulf commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2401
 
    As a user, I think the first alternative is the clearest, it explicitly states the intention and does not change any existing behavior (i.e. auto tune -1 which we already rely on). In our case, only trusted users can send messages to management, so we are willing to take the 'risk'.


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

[GitHub] activemq-artemis issue #2401: ARTEMIS-1710 Allow management msgs to pass the...

jbertram-2
In reply to this post by jbertram-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2401
 
    @clebertsuconic I'm fixing a couple of things on this one too


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

[GitHub] activemq-artemis issue #2401: ARTEMIS-1710 Allow management msgs to pass the...

jbertram-2
In reply to this post by jbertram-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2401
 
    @clebertsuconic I have added some additional unit tests on `PagingStoreImpl`


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

[GitHub] activemq-artemis issue #2401: ARTEMIS-1710 Allow management msgs to pass the...

jbertram-2
In reply to this post by jbertram-2
Github user mtaylor commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2401
 
    I don't really like the idea of the double threshold.  I think the global-max-size limit should be the catch all case.  
   
    What would work better here is an  accumulative-max-size setting that applies to an address space that limits the total amount of memory used by all addresses that match that address setting.  Right now the limits are on a per address basis, but many addresses can match the same address space.  
   
    Doing it this way allows a lot more flexibility in the broker, users can limit certain address spaces for other reasons that to enable management messages to flow, like for example, reserving capacity for critical applications and blocking others.  e.g. telemetry.# vs command.#


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

[GitHub] activemq-artemis issue #2401: ARTEMIS-1710 Allow management msgs to pass the...

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

    https://github.com/apache/activemq-artemis/pull/2401
 
    This would work and i like the flexibility. I assume here in this option proposed. Global would remain as is today.


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

[GitHub] activemq-artemis issue #2401: ARTEMIS-1710 Allow management msgs to pass the...

jbertram-2
In reply to this post by jbertram-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2401
 
    @mtaylor @michaelandrepearce I can't ignore all these useful comments/feedbacks guys so I will work more on this one to see if it can be done in a better way; thanks for the reviews :+1:


---
123