[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Pass ServerConsumer to messageE...

[michaelandrepearce]

Github user michaelandrepearce commented on the issue:

https://github.com/apache/activemq-artemis/pull/2012
  
I personally prefer this. 


---

[GitHub] activemq-artemis issue #2011: ARTEMIS-1740: Add support for regex based cert...

[clebertsuconic]

Github user clebertsuconic commented on the issue:

https://github.com/apache/activemq-artemis/pull/2011
  
done


---

[GitHub] activemq-artemis pull request #2011: ARTEMIS-1740: Add support for regex bas...

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] activemq-artemis issue #2011: ARTEMIS-1740: Add support for regex based cert...

[clebertsuconic]

Github user clebertsuconic commented on the issue:

https://github.com/apache/activemq-artemis/pull/2011
  
@LionelCons I can.. if this is ready to be merged.


---

[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Pass ServerConsumer to messageE...

[cshannon]

Github user cshannon commented on the issue:

https://github.com/apache/activemq-artemis/pull/2012
  
@franz1981 and @michaelandrepearce  - I updated the patch to pass a 
reference to the ServerConsumer (when it applies) for both the expired and 
acked callbacks and dropped the extra reference to the serverId in the 
MessageReference classlet me know what you think


---

Re: Not able to start connection with ActiveMQ broker from a network client.

[LeonelDuran]

Thanks Tim. 

you know Why does this happen?, the error occurs in a single cluster node
and the configurtion was the same please help me 



--
Sent from: http://activemq.2283324.n4.nabble.com/ActiveMQ-Dev-f2368404.html

[GitHub] activemq-artemis issue #2011: ARTEMIS-1740: Add support for regex based cert...

[LionelCons]

Github user LionelCons commented on the issue:

https://github.com/apache/activemq-artemis/pull/2011
  
@jbertram can you squash the commits when merging?


---

[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...

[michaelandrepearce]

Github user michaelandrepearce commented on the issue:

https://github.com/apache/activemq-artemis/pull/2012
  
My point is whilst it will require not quite a quick win, it’s not 
impossible to avoid this, and refactor to get this is probably a good thing. I 
realise it’s not a quick win as this. But nor is all the scaling and perf 
Work we have been doing.


---

[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...

[franz1981]

Github user franz1981 commented on the issue:

https://github.com/apache/activemq-artemis/pull/2012
  
@cshannon 
Summarized:
```
org.apache.activemq.artemis.core.server.impl.MessageReferenceImpl:
Instance size: 72 bytes
Space losses: 2 bytes internal + 4 bytes external = 6 bytes total

org.apache.activemq.artemis.core.paging.cursor.PagedReferenceImpl:
Instance size: 104 bytes
Space losses: 1 bytes internal + 4 bytes external = 5 bytes total

org.apache.activemq.artemis.core.server.impl.LastValueQueue$HolderReference:
Instance size: 40 bytes
Space losses: 3 bytes internal + 0 bytes external = 3 bytes total
```
`Instance size` is the value you're searching for: I have attached the 
`Space losses` as well, because it
is related to what @michaelandrepearce is saying: just adding that mere 4 
bytes reference (with heap < 32 Gb the JVM uses compressed oops by default) has 
added 4 bytes (external) of padding ie a total of 8 bytes of more space used.
TBH that's a difficult choice: we have done recently many changes to make 
every protocol much GC "gentle" to allow scaling more easily, but @cshannon is 
right that there are monitoring/telemetry reasons very important to be achieved 
as well.
You both have very good points :O





---

[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...

[michaelandrepearce]

Github user michaelandrepearce commented on the issue:

https://github.com/apache/activemq-artemis/pull/2012
  
I disagree it’s a 4% overhead at best and 10% extra over head for worst. 
That’s quite a kick


---

[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...

[cshannon]

Github user cshannon commented on the issue:

https://github.com/apache/activemq-artemis/pull/2012
  
I understand wanting to keep memory low but there's a point where you take 
things too far.  At some point usability and correctness is more important.  
Having just a consumerId is just not correct, end of story.  It is not a unique 
value and will be duplicated. The client library is responsible for creating 
that consumerId per session so there's no way to go back and make the 
consumerId unique (can't change old client libraries).  I wish there were, that 
would solve this problem.

I do not agree that refactoring is the best approach.  At the end of the 
day if we can't add a reference to a String I think we are taking trying to 
limit memory usage too far.


---

[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...

[michaelandrepearce]

Github user michaelandrepearce commented on the issue:

https://github.com/apache/activemq-artemis/pull/2012
  
As noted my biggest concern is that message refs need to be as light as 
humanly possible as they’re all in memory and affects greatly the scaling.

I would personally prefer the refactor if needed, than take this hit. 

Especially as this is only needed by someone wanting to use this in a 
plugin. Which means everyone else has to suffer


---

[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...

[cshannon]

Github user cshannon commented on the issue:

https://github.com/apache/activemq-artemis/pull/2012
  
@michaelandrepearce  - because 1) there can be more than just that use case 
for having sessionId part of the reference in the future and 2) the acknowledge 
code is not part of the consumer and is handled later by the QueueImpl...see 
the acknowledge method inside QueueImpl..it just gets the reference and does 
the acking and doesn't know the consumer by that point because the 
ServerConsumer calls ack on the ref but doesn't pass itself to it...there would 
have to be a good amount of refactoring to change this along with changes to 
public interfaces


---

[GitHub] activemq-artemis issue #2011: ARTEMIS-1740: Add support for regex based cert...

[jbertram]

Github user jbertram commented on the issue:

https://github.com/apache/activemq-artemis/pull/2011
  
Thanks for the changes, @LionelCons. Can you squash your commits?


---

[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...

[michaelandrepearce]

Github user michaelandrepearce commented on the issue:

https://github.com/apache/activemq-artemis/pull/2012
  
Should that then not be logged by the consumer code referencing the message 
if needed? As the consumer anyhow will be causing the ack.




---

[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...

[cshannon]

Github user cshannon commented on the issue:

https://github.com/apache/activemq-artemis/pull/2012
  
For example, it's a common use case to fire notifications or logging when a 
message is acknowledged.  I have a requirement to do this for my organization 
and I need to quickly track the specific consumer that acked the message from 
inside the plugin.  Having the sessionId as part of the reference is the only 
way to do this.


---

[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...

[cshannon]

Github user cshannon commented on the issue:

https://github.com/apache/activemq-artemis/pull/2012
  
@franz1981 - So what size would you recommend? Seems like maybe 8 is too 
much to add to the estimation based on the output.


---

[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...

[cshannon]

Github user cshannon commented on the issue:

https://github.com/apache/activemq-artemis/pull/2012
  
Furthermore there is more to a broker than just processing.  Monitoring and 
metrics are very important to business flows and trying to save a few bytes of 
memory is not worth it if you sacrifice basic things such as being able to do 
proper logging and troubleshooting.


---

[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...

[cshannon]

Github user cshannon commented on the issue:

https://github.com/apache/activemq-artemis/pull/2012
  
It's not attached to the message.  The reference has the information for 
which consumer is attached to it.  The consumer id is not unique so you need to 
have the session Id as well otherwise there is no way to find out which 
consumer the reference belongs to so this has to be in memory.

I really don't think it's going to be a big deal to add a reference id.  


---

[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...

[franz1981]

Github user franz1981 commented on the issue:

https://github.com/apache/activemq-artemis/pull/2012
  
@michaelandrepearce Michael have good points on that, especially for such 
important classes.
@cshannon I have anyway the results:
```
# Running 64-bit HotSpot VM.
# Using compressed oop with 3-bit shift.
# Using compressed klass with 3-bit shift.
# Objects are 8 bytes aligned.
# Field sizes by type: 4, 1, 1, 2, 2, 4, 4, 8, 8 [bytes]
# Array element sizes: 4, 1, 1, 2, 2, 4, 4, 8, 8 [bytes]

org.apache.activemq.artemis.core.server.impl.MessageReferenceImpl object 
internals:
 OFFSET  SIZE   
 TYPE DESCRIPTION  VALUE
  012   
  (object header)  N/A
 12 4   
  int Node.iterCount   N/A
 16 4   
org.apache.activemq.artemis.utils.collections.LinkedListImpl.Node Node.next 
   N/A
 20 4   
org.apache.activemq.artemis.utils.collections.LinkedListImpl.Node Node.prev 
   N/A
 24 8   
 long MessageReferenceImpl.scheduledDeliveryTime   N/A
 32 8   
 long MessageReferenceImpl.consumerID  N/A
 40 4   
  int MessageReferenceImpl.deliveryCount   N/A
 44 4   
  int MessageReferenceImpl.persistedCount  N/A
 48 1 
boolean MessageReferenceImpl.hasConsumerID   N/A
 49 1 
boolean MessageReferenceImpl.alreadyAckedN/A
 50 2   
  (alignment/padding gap) 
 52 4
org.apache.activemq.artemis.api.core.Message MessageReferenceImpl.message   
  N/A
 56 4   
org.apache.activemq.artemis.core.server.Queue MessageReferenceImpl.queue
   N/A
 60 4
java.lang.String MessageReferenceImpl.sessionId   N/A
 64 4
java.lang.Object MessageReferenceImpl.protocolDataN/A
 68 4   
  (loss due to the next object alignment)
Instance size: 72 bytes
Space losses: 2 bytes internal + 4 bytes external = 6 bytes total

*
org.apache.activemq.artemis.core.paging.cursor.PagedReferenceImpl object 
internals:
 OFFSET  SIZE   
 TYPE DESCRIPTION   VALUE
  012   
  (object header)   N/A
 12 4   
  int Node.iterCountN/A
 16 4   
org.apache.activemq.artemis.utils.collections.LinkedListImpl.Node Node.next 
N/A
 20 4   
org.apache.activemq.artemis.utils.collections.LinkedListImpl.Node Node.prev 
N/A
 24 8   
 long PagedReferenceImpl.deliveryTime   N/A
 32 8   
 long PagedReferenceImpl.consumerID N/A
 40 8   
 long PagedReferenceImpl.transactionID  N/A
 48 8   
 long PagedReferenceImpl.messageID  N/A
 56 8   
 long PagedReferenceImpl.messageSizeN/A
 64 4   
  int PagedReferenceImpl.persistedCount N/A
 68 4   
  int PagedReferenceImpl.messageEstimateN/A
 72 4   
  int PagedReferenceImpl.deliveryCount  N/A
 76 1 

[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...

[michaelandrepearce]

Github user michaelandrepearce commented on the issue:

https://github.com/apache/activemq-artemis/pull/2012
  
Sorry to be a numpty but why this needs to be held in memory? Is it going 
to be used for processing the flow in broker.

From an admin point even if the message is paged, if a browser or admin 
needs it simply get it from the message (reading out of paging) 

Less stuff having to hold in memory better perf.

I’m a little against this if there’s no processing need for this. 


---

[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...

[franz1981]

Github user franz1981 commented on the issue:

https://github.com/apache/activemq-artemis/pull/2012
  
@cshannon Sure, np! Probably I have some minutes to do it now ;) Just a 
matter to download your branch and I will do it


---

[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...

[cshannon]

Github user cshannon commented on the issue:

https://github.com/apache/activemq-artemis/pull/2012
  
@franz1981 - If you don't mind double checking with the tool again I would 
appreciate it since you have already validated the memory before but if you 
don't have time I can try and figure it out


---

[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...

[franz1981]

Github user franz1981 commented on the issue:

https://github.com/apache/activemq-artemis/pull/2012
  
@cshannon I've validated the original value using this tool: 
http://openjdk.java.net/projects/code-tools/jol/
If possible do the same or just wait that I take a look using it as well 
using the default configuration used for the broker.


---

[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...

[cshannon]

Github user cshannon commented on the issue:

https://github.com/apache/activemq-artemis/pull/2012
  
I updated the PR and added 8 to the estimated size in MessageRefereceImpl.


---

[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...

[franz1981]

Github user franz1981 commented on the issue:

https://github.com/apache/activemq-artemis/pull/2012
  
@cshannon ehehe I wish to have said "would" :) The problem with the 
references is that the footprint of the reference depends on the JVM 
configuration (`-XX:+UseCompressedOops` or not) and on the alignment with the 
rest of the fields: the JVM tends to add padding in order to have each field 
size-aligned.


---

[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...

[cshannon]

Github user cshannon commented on the issue:

https://github.com/apache/activemq-artemis/pull/2012
  
@franz1981 - Did you mean probably "would" be enough? The string is a UUID 
(which is normally 16 bytes I think) but it is owned by the ServerSessionImpl 
so it would just be a reference so however much memory that takes up (maybe 8 
bytes to be safe?)


---

[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...

[franz1981]

Github user franz1981 commented on the issue:

https://github.com/apache/activemq-artemis/pull/2012
  
@cshannon It would be needed to update the memory footprint estimation of 
each message as well: rising it up it by 4 or 8 bytes wouldn't be enough 
probably :+1: 


---

[GitHub] activemq-artemis pull request #2012: ARTEMIS-1803 - Add sessionId to Message...

[cshannon]

GitHub user cshannon opened a pull request:

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

ARTEMIS-1803 - Add sessionId to MessageReference

Track the sessionId along with the consumerId in a MessageReference when
appropriate in order to figure out which consumer the reference belongs
to

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

$ git pull https://github.com/cshannon/activemq-artemis ARTEMIS-1803

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

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


commit 262cac9f0b8b581b56bda6219bd43b7922edf6c9
Author: Christopher L. Shannon (cshannon) 
Date:   2018-04-12T11:05:16Z

ARTEMIS-1803 - Add sessionId to MessageReference

Track the sessionId along with the consumerId in a MessageReference when
appropriate in order to figure out which consumer the reference belongs
to




---

[GitHub] activemq-artemis issue #2011: ARTEMIS-1740: Add support for regex based cert...

[LionelCons]

Github user LionelCons commented on the issue:

https://github.com/apache/activemq-artemis/pull/2011
  
The final code can be seen 
[here](https://github.com/LionelCons/activemq-artemis/blob/9ead970395975a28846de4ba237eb78bc728c8fb/artemis-server/src/main/java/org/apache/activemq/artemis/spi/core/security/jaas/TextFileCertificateLoginModule.java).

Just like in the original code, both `getUserNameForCertificates` and 
`getUserRoles` still use `get`.


---

[GitHub] activemq-artemis pull request #2011: ARTEMIS-1740: Add support for regex bas...

[franz1981]

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

https://github.com/apache/activemq-artemis/pull/2011#discussion_r181025545
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/spi/core/security/jaas/TextFileCertificateLoginModule.java
 ---
@@ -71,8 +79,12 @@ protected String getUserNameForCertificates(final 
X509Certificate[] certs) throw
   if (certs == null) {
  throw new LoginException("Client certificates not found. Cannot 
authenticate.");
   }
-
-  return usersByDn.get(getDistinguishedName(certs));
+  String dn = getDistinguishedName(certs);
+  String name = usersByDn.get(dn);
--- End diff --

Hence this `userByDb` not synchronized access no longer exists in the new 
commit?
With github and multiple commits is not simple to do reviews :P


---

[GitHub] activemq-artemis issue #2011: ARTEMIS-1740: Add support for regex based cert...

[LionelCons]

Github user LionelCons commented on the issue:

https://github.com/apache/activemq-artemis/pull/2011
  
@franz1981 your comment goes beyond my modifications since the existing 
code already uses the `get` method (in `getUserNameForCertificates` and 
`getUserRoles`, at least).


---

[GitHub] activemq-artemis issue #2011: ARTEMIS-1740: Add support for regex based cert...

[franz1981]

Github user franz1981 commented on the issue:

https://github.com/apache/activemq-artemis/pull/2011
  
@LionelCons 

And it makes sense, but there are parts like:
`String name = usersByDn.get(dn);`
that are not synchronized, hence it won't be thread-safe.
My advice is to use a lazy initialization using a `Suppliers::memoize` or 
similar construct and always use `Supplier::get` to access the variable: that 
would allow to have always a thread-safe access to an unmodifiable resource, 
lazy initialized.



---

[GitHub] activemq-artemis issue #2011: ARTEMIS-1740: Add support for regex based cert...

[LionelCons]

Github user LionelCons commented on the issue:

https://github.com/apache/activemq-artemis/pull/2011
  
The intent of  `synchronized` is to protect the modification of `usersByDn`.


---

[GitHub] activemq-artemis pull request #2011: ARTEMIS-1740: Add support for regex bas...

[franz1981]

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

https://github.com/apache/activemq-artemis/pull/2011#discussion_r180998219
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/spi/core/security/jaas/TextFileCertificateLoginModule.java
 ---
@@ -71,8 +79,12 @@ protected String getUserNameForCertificates(final 
X509Certificate[] certs) throw
   if (certs == null) {
  throw new LoginException("Client certificates not found. Cannot 
authenticate.");
   }
-
-  return usersByDn.get(getDistinguishedName(certs));
+  String dn = getDistinguishedName(certs);
+  String name = usersByDn.get(dn);
+  if (name == null && regexpByUser != null) {
+ name = getUserByRegexp(dn);
--- End diff --

`getUserByRegexp` is synchronized but there are uses of `usersByDn` and 
`regexpByUser`, like these ones, that are not: what is the reason to have 
`String getUserByRegexp(String dn)` synchronized?


---

[GitHub] activemq-artemis pull request #2011: ARTEMIS-1740: Add support for regex bas...

[franz1981]

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

https://github.com/apache/activemq-artemis/pull/2011#discussion_r180996871
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/spi/core/security/jaas/ReloadableProperties.java
 ---
@@ -95,6 +99,21 @@ public synchronized ReloadableProperties obtained() {
   return invertedValueProps;
}
 
+   public synchronized Map regexpPropertiesMap() {
--- End diff --

You could use a 
[Supplier::memoize](https://google.github.io/guava/releases/19.0/api/docs/com/google/common/base/Suppliers.html#memoize(com.google.common.base.Supplier)
 to allow a thread-safe lazy initialization without having that method 
synchronized even when you just need to get `regexpProps`


---

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

[stanlyDoge]

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

https://github.com/apache/activemq-artemis/pull/2010#discussion_r180976916
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
 ---
@@ -2727,7 +2731,7 @@ private Message makeCopy(final MessageReference ref,
   Message copy = message.copy(newID);
 
   if (copyOriginalHeaders) {
- copy.referenceOriginalMessage(message, ref != null ? 
ref.getQueue().getName().toString() : null);
+ copy.referenceOriginalMessage(message, 
ref.getQueue().getName().toString());
--- End diff --

@clebertsuconic thank you :) Can you please check again? Especially null 
reference logging. 


---