[GitHub] activemq-artemis issue #2468: Add CR in sanity check

2018-12-19 Thread BiNZGi 
Github user BiNZGi commented on the issue:

https://github.com/apache/activemq-artemis/pull/2468
  
Thank you for the hint with JIRA. Today I have made some more tests and 
recognized that there was a mismatch with the STOMP versions in my case. The 
messages were in STOMP 1.1 but with CR that is not allowed in this version.
Therefore I close this PR.


---

[GitHub] activemq-artemis issue #2468: Add CR in sanity check

2018-12-19 Thread jbertram 
Github user jbertram commented on the issue:

https://github.com/apache/activemq-artemis/pull/2468
  
After looking at this a bit more closely I think this change is incorrect. 
The optional carriage return (octet 13) was added in STOMP 1.2, but you have 
changed `org.apache.activemq.artemis.core.protocol.stomp.StompDecoder` which is 
responsible for STOMP 1.0 frames. There are different decoder implementations 
for STOMP 1.1 & 1.2, and from what I can tell the 1.2 decoder handles the EOL 
properly. In fact, the STOMP client used by the test suite sends `\r\n` EOLs by 
default when using STOMP 1.2. 

Unless you have a test demonstrating a problem I think this PR should be 
closed.


---

[GitHub] activemq-artemis issue #2468: Add CR in sanity check

2018-12-18 Thread jbertram 
Github user jbertram commented on the issue:

https://github.com/apache/activemq-artemis/pull/2468
  
Could you create a JIRA for this at 
https://issues.apache.org/jira/projects/ARTEMIS and also add a test to 
reproduce the failure you were seeing without the fix? Something simple in 
`org.apache.activemq.artemis.tests.integration.stomp.StompTest` should suffice.


---