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

[franz1981]

Github user franz1981 commented on the issue:

https://github.com/apache/activemq-artemis/pull/2401
  
@clebert I will try to connect later in order to understand what you mean 
mate (ATM I'm on the phone)


---

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

[clebertsuconic]

Github user clebertsuconic commented on the issue:

https://github.com/apache/activemq-artemis/pull/2401
  
@lulf / @franz1981  ok, 

Why not make a simpler change.. and return -1 on PaginManager if the 
management address. That's a lot less code to get around the sizing.


@franz1981 Also, please rename the commit and JIRA to *exceed* size. 


---

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

[jbertram]

Github user jbertram commented on the issue:

https://github.com/apache/activemq-artemis/pull/2401
  
FWIW, management can be done through JMX or REST (via Jolokia) if 
management messages are blocked for any reason.


---

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

[franz1981]

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

https://github.com/apache/activemq-artemis/pull/2401#discussion_r229845531
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/config/Configuration.java
 ---
@@ -493,6 +493,17 @@ Configuration addDiscoveryGroupConfiguration(String 
key,
 */
Configuration setManagementAddress(SimpleString address);
 
+   /**
+* Sets whether {@link #getManagementAddress()} ignores Global Max Size 
limit.
+*/
+   Configuration setManagementAddressIgnoreGlobalMaxSize(boolean value);
+
+   /**
+* Returns {@code true} if {@link #getManagementAddress()} ignores 
Global Max Size limit.
+*/
+   boolean isManagementAddressIgnoreGlobalMaxSize();
--- End diff --

If you set the max size of an address to -1 the broker will fallback to use 
global-max-size instead AFIK


---

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

[lulf]

Github user lulf commented on the issue:

https://github.com/apache/activemq-artemis/pull/2401
  
The use case is covered in 
https://issues.apache.org/jira/browse/ARTEMIS-1710

In a cloud-setting, we need to:

a) Have the broker derive (global) limits based on things like heap memory 
in order to avoid falling over. This may be configured statically in the 
broker.xml
b) Allow management messages to get through even if other addresses are 
blocked by global-max-size (using BLOCK/FAIL policy, we do not wish to use 
PAGE) so that our system can keep operating even if queues are full (i.e. we 
can fetch queue stats and report back to user always)

Also, keep in mind that:

a) We don't know what addresses will exist beforehand and they are created 
via management address.
b) We want to avoid setting limits per address via management, as that gets 
super slow (I know this was confirmed by someone a while back, i don't know the 
jira number). Perhaps the address-space limits proposed by @mtaylor would work 
around that allowing us to set it statically for all addresses and make 
management an exception in our configuration. I'm not sure how that would work 
with not knowing the limits beforehand, or if it would be possible to 'disable' 
the limit for management addresses by setting it to some special value.


---

[GitHub] activemq-artemis pull request #2369: ARTEMIS-2123 Paging not stopped if ther...

[asfgit]

Github user asfgit closed the pull request at:

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


---

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

[clebertsuconic]

Github user clebertsuconic commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2401#discussion_r229835269
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/config/Configuration.java
 ---
@@ -493,6 +493,17 @@ Configuration addDiscoveryGroupConfiguration(String 
key,
 */
Configuration setManagementAddress(SimpleString address);
 
+   /**
+* Sets whether {@link #getManagementAddress()} ignores Global Max Size 
limit.
+*/
+   Configuration setManagementAddressIgnoreGlobalMaxSize(boolean value);
+
+   /**
+* Returns {@code true} if {@link #getManagementAddress()} ignores 
Global Max Size limit.
+*/
+   boolean isManagementAddressIgnoreGlobalMaxSize();
--- End diff --

Why not simply configure the management address as -1, which would be 
unlimited?


---

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

[clebertsuconic]

Github user clebertsuconic commented on the issue:

https://github.com/apache/activemq-artemis/pull/2401
  
@franz1981 Did you mean exceed global-max-size?

when I read this originally I had the impression you were setting 
global-max-size using management messages.

I'm still trying to get my head around basic use cases.. such as.. why not 
simply set the management address size as -1? 

I'm confused on basic stuff here.


---

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

[mtaylor]

Github user mtaylor 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.

Yes. 


---

[GitHub] activemq-artemis pull request #2408: ARTEMIS-2160: Addressed occurance where...

[RoddieKieley]

GitHub user RoddieKieley opened a pull request:

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

ARTEMIS-2160: Addressed occurance where cluster configuration on serv…

…er locator was hard coded. Covered with test.

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

$ git pull https://github.com/RoddieKieley/activemq-artemis ARTEMIS-2160

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

https://github.com/apache/activemq-artemis/pull/2408.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 #2408


commit 2198ab8c1130133f503273860b8a97cceb7a8084
Author: Roddie Kieley 
Date:   2018-10-31T17:34:07Z

ARTEMIS-2160: Addressed occurance where cluster configuration on server 
locator was hard coded. Covered with test.




---

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

[franz1981]

Github user franz1981 commented on the issue:

https://github.com/apache/activemq-artemis/pull/2401
  
@michaelandrepearce @lulf @mtaylor I will continue on the issue comments 
:+1: 


---

[GitHub] activemq-artemis pull request #2407: openWire would allow one extra send

[asfgit]

Github user asfgit closed the pull request at:

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


---

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

[franz1981]

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: 


---

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

[michaelandrepearce]

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.


---

[GitHub] activemq-artemis issue #2406: New profile to allow skipping or including the...

[orpiske]

Github user orpiske commented on the issue:

https://github.com/apache/activemq-artemis/pull/2406
  
> how to run a single test:
> 
> ```
> mvn -Ptests -DfailIfNoTests=false -Pextra-tests -DskipStyleCheck=true 
-DskipPerformanceTests=false -Dtest=$1 test
> ```
> 
> ^^^ the following is provided as ./scripts/one-test.sh from main.
> 
> and TBH: most people just use the IDE to start a single test.
> 
> or if you just want to run the integration-tests, you can call mvn 
-Ptests within integration-tests
> 
> I really don't see a reason for this profile.

The primary reason is that they want to run a couple of tests (without the 
integration ones) on the CI. 

That said: I see no reason to struggle with this without having exhausted 
other possibilities. I will close this PR and will point them to this as a 
source of some ideas. If nothing comes up that satisfies them, then we can re 
discuss this on better grounds.


---

[GitHub] activemq-artemis pull request #2406: New profile to allow skipping or includ...

[orpiske]

Github user orpiske closed the pull request at:

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


---

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

[mtaylor]

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


---

[GitHub] activemq-artemis pull request #2400: ARTEMIS-2151 JMS Selectors broken in so...

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] activemq-artemis pull request #2404: ARTEMIS-2157 Extra information on Criti...

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] activemq-artemis issue #2406: New profile to allow skipping or including the...

[clebertsuconic]

Github user clebertsuconic commented on the issue:

https://github.com/apache/activemq-artemis/pull/2406
  
how to run a single test:


```
mvn -Ptests -DfailIfNoTests=false -Pextra-tests -DskipStyleCheck=true 
-DskipPerformanceTests=false -Dtest=$1 test
```

^^^ the following is provided as ./scripts/one-test.sh from main.


and TBH: most people just use the IDE to start a single test.

or if you just want to run the integration-tests, you can call mvn -Ptests 
within integration-tests


I really don't see a reason for this profile.



---

[GitHub] activemq-artemis issue #2406: New profile to allow skipping or including the...

[clebertsuconic]

Github user clebertsuconic commented on the issue:

https://github.com/apache/activemq-artemis/pull/2406
  
There is a script. One-test.sh.  Or you can run the test in an ide. 


---

[GitHub] activemq-artemis issue #2406: New profile to allow skipping or including the...

[orpiske]

Github user orpiske commented on the issue:

https://github.com/apache/activemq-artemis/pull/2406
  
We have tried the proposed approach and it did not skip the integration 
tests. We received a suggestion from @andytaylor to use "-PDev" in addition to 
that. Any solution that does not involve code changes would be preferred, 
indeed.


---

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

[franz1981]

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`


---

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

[GitHub] activemq-artemis issue #2407: openWire would allow one extra send

[orpiske]

Github user orpiske commented on the issue:

https://github.com/apache/activemq-artemis/pull/2407
  
Looking at the test, I think it correctly reproduces the problem we were 
seeing. Thanks for the quick fix!


---

[GitHub] activemq-artemis pull request #2405: ARTEMIS-2158 don't get pagedMessage if ...

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] activemq-artemis issue #2406: New profile to allow skipping or including the...

[clebertsuconic]

Github user clebertsuconic commented on the issue:

https://github.com/apache/activemq-artemis/pull/2406
  
This is not needed at all. can you close it?


---

[GitHub] activemq-artemis pull request #2407: openWire would allow one extra send

[clebertsuconic]

GitHub user clebertsuconic opened a pull request:

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

openWire would allow one extra send



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

$ git pull https://github.com/clebertsuconic/activemq-artemis ARTEMIS-2159

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

https://github.com/apache/activemq-artemis/pull/2407.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 #2407


commit d1fd86ece23bc499c54b295316bcd94494a764ad
Author: Otavio Rodolfo Piske 
Date:   2018-10-31T11:10:52Z

ARTEMIS-2159 Reproducer test that demonstrates that it is possible to force 
the broker
to accept messages even after the queue is full and an exception is raised

commit 2ab0f7b0eeb3e7e05e784646839580354dbf62cb
Author: Clebert Suconic 
Date:   2018-10-31T13:13:05Z

ARTEMIS-2159 OpenWire would allow one extra send




---

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

[franz1981]

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


---

[GitHub] activemq-artemis pull request #:

[orpiske]

Github user orpiske commented on the pull request:


https://github.com/apache/activemq-artemis/commit/9490e745c5431e681c768e6fdd20ed5dda0b7b95#commitcomment-31119234
  
@andytaylor I didn't know about that one! I think that might do the trick. 
We'll try and I'll close if it solves the issue.


---

[GitHub] activemq-artemis pull request #:

[andytaylor]

Github user andytaylor commented on the pull request:


https://github.com/apache/activemq-artemis/commit/9490e745c5431e681c768e6fdd20ed5dda0b7b95#commitcomment-31119169
  
Im not sure this is needed, you can do -DskipIntegrationTests=true


---

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

[lulf]

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


---

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

[franz1981]

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
Any value or left it -1 to be auto-tuned

true
```
the alternative solution will need to configure:
 ```xml
The original value used for 
global-max-size
A value that is assumed to be unlimited
```
 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


---

[GitHub] activemq-artemis pull request #2406: New profile to allow skipping or includ...

[orpiske]

GitHub user orpiske opened a pull request:

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

New profile to allow skipping or including the execution of integration 
tests

This comes as request from our team. They would like to be able to skip 
and/or include the execution of the integration tests on demand.

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

$ git pull https://github.com/orpiske/activemq-artemis master

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

https://github.com/apache/activemq-artemis/pull/2406.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 #2406


commit 9490e745c5431e681c768e6fdd20ed5dda0b7b95
Author: Otavio Rodolfo Piske 
Date:   2018-10-31T08:40:38Z

Create a new profile to allow skipping or including the execution of 
integration tests




---

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

[franz1981]

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.


---

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

[franz1981]

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?


---

[GitHub] activemq-artemis issue #2400: ARTEMIS-2151 JMS Selectors broken in some case...

[michaelandrepearce]

Github user michaelandrepearce commented on the issue:

https://github.com/apache/activemq-artemis/pull/2400
  
For openwire JMSPriority, JMSCorrelationID etc etc were all broken.


---