[GitHub] activemq-artemis pull request #2418: ARTEMIS-2142 Fix ServerJMSMessage

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

[GitHub] activemq-artemis pull request #2418: ARTEMIS-2142 Fix ServerJMSMessage

jbertram-2
GitHub user michaelandrepearce opened a pull request:

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

    ARTEMIS-2142 Fix ServerJMSMessage

    Fix ServerJMSMessage so it correctly transposes JMSX headers.
    Push common JMS to Message Interface mappings into MessageUtil

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

    $ git pull https://github.com/michaelandrepearce/activemq-artemis ARTEMIS-2142-EXTRA

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

    https://github.com/apache/activemq-artemis/pull/2418.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 #2418
   
----
commit 8adcfd4478f4338bba0799cf4e42a2b935ce9bd1
Author: Michael André Pearce <michael.andre.pearce@...>
Date:   2018-11-06T21:30:52Z

    ARTEMIS-2142 Fix ServerJMSMessage
   
    Fix ServerJMSMessage so it correctly transposes JMSX headers.
    Push common JMS to Message Interface mappings into MessageUtil

----


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

[GitHub] activemq-artemis issue #2418: ARTEMIS-2142 Fix ServerJMSMessage

jbertram-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2418
 
    @gemmellr this should sort the failures you're seeing out.


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

[GitHub] activemq-artemis issue #2418: ARTEMIS-2142 Fix ServerJMSMessage

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

    https://github.com/apache/activemq-artemis/pull/2418
 
    I tried the tests and they worked, but it occurred to me that though changes might be needed in general to fix whats currently stopping the tests passing, just making them pass is likely masking another issue that is the actual problem.
   
    I had a look what one of the tests is actually doing with JMSXGroupSeq given it worked before and didnt now. The answer seems to be nothing at all, which is somewhat expected given its not testing that. As such the failure to handle JMSXGroupSeq properly isnt the main issue, the fact its present at all would be. If nothing set it then it shouldn't be present. With this change in place I can see the received AMQP message in one of the previously-failing tests does have a group sequence value (of 0) populated when it seemingly should not.
   
    More of an aside, I'm not hugely fond of 'getValidatedUserId' as a name since its not clear anything has actually validated it. The various setters added to AMQPMessage perhaps also open up scope for protocol violations (since they are all immutable properties).


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

[GitHub] activemq-artemis issue #2418: ARTEMIS-2142 Fix ServerJMSMessage

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

    https://github.com/apache/activemq-artemis/pull/2418
 
    So the issue was sequence wasnt being transposed correctly also found other jmsx ones werent in doing so. But just been under the cover. Thus what we fix here.
   
    Re the name this is the name in the message interface that already existed for that.


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

[GitHub] activemq-artemis issue #2418: ARTEMIS-2142 Fix ServerJMSMessage

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

    https://github.com/apache/activemq-artemis/pull/2418
 
    > Re seq not set, its an int field so its tech always set if you request it, just should default 0 if not set.
   
    JMSXGroupSeq is set explicitly by the application. If it isn't set by the application, then the property doesn't exist, the same as any other int property. It shouldn't be in the property names and we shouldn't be setting group-sequence to 0 on the wire when nothing has actually set JMSXGroupSeq.
   
    > Re the name this is the name in the message interface that already existed for that.
   
    Oh, right. Yes, according to the changelog adding it, it deliberately wasn't implemented in the AMQPMessage, which makes sense because per my earlier comment using it would be a protocol violation given it is an immutable part of the message. So that bit should be unwound I think.
   
    > If i was just to fix the test, it would have been a one liner null check , and i didnt want to just fix the test (would have been less work for me too if i did that). But the fact is as i mentioned above finding a few things that better to fix it all up a bit, thus why its a bit more involved change
   
    I think a one-liner fix would probably have been better, keeping the general improvement separate/later. It makes it easier to see the actual issue and fix is, and makes backporting bug fixes easier without extra changes either being in the way or tagging along (not that that actually applies here specifically, but generally I mean).
   
    As I've said though, it doesnt seem that either actually addresses the apparent main issue though, of group sequence values being present in messages a sequence was never set on.


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

[GitHub] activemq-artemis issue #2418: ARTEMIS-2142 Fix ServerJMSMessage

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

    https://github.com/apache/activemq-artemis/pull/2418
 
    So openwire always returns a value on asking groupseq. It never gives an ability to determine if it was set by client or not.


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

[GitHub] activemq-artemis issue #2418: ARTEMIS-2142 Fix ServerJMSMessage

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

    https://github.com/apache/activemq-artemis/pull/2418
 
    Re:
   
    Oh, right. Yes, according to the changelog adding it, it deliberately wasn't implemented in the AMQPMessage, which makes sense because per my earlier comment using it would be a protocol violation given it is an immutable part of the message. So that bit should be unwound I think.
   
    We have to be able to set it somehow. This is being set on messags conversion. The issues here are on protocol conversion. Were not changing amqp message


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

[GitHub] activemq-artemis issue #2418: ARTEMIS-2142 Fix ServerJMSMessage

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

    https://github.com/apache/activemq-artemis/pull/2418
 
    I would much rather we fix this proper of your adament for a sticky plaster then fine. But i see doing that being problematic longer run as we just keep hiding underlying issues


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

[GitHub] activemq-artemis issue #2418: ARTEMIS-2142 Fix ServerJMSMessage

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

    https://github.com/apache/activemq-artemis/pull/2418
 
    I can add tests that would show the other breaks i found. That a sticky plaster null check solution wouldnt fix.


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

[GitHub] activemq-artemis issue #2418: ARTEMIS-2142 Fix ServerJMSMessage

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

    https://github.com/apache/activemq-artemis/pull/2418
 
    > So openwire always returns a value on asking groupseq. It never gives an ability to determine if it was set by client or not.
   
    It does seem to return a value if you ask for one, which it shouldn't, but it also does not populate the property name all the time, so its not clear why the value would be asked for. This is showing up in the AMQP message because it is in the property names being iterated, which it shouldn't be.
   
    > We have to be able to set it somehow during conversion. This is being set on message conversion. The issues here are on protocol conversion. Were not changing amqp message
   
    From what I see, the method is not used during conversion to an AMQP message, with that being set directly on the underlying Properties section within the converter. Setting it any other time is then stepping toward a protocol violation.
   
    I am not asking for just a sticking plaster. I did suggest that we fix small "one line" bugs separately from comparatively massive general code refactorings (did not say they shouldnt be done). I suggest that we avoid actively violating the protocol as this likely will likely result (and as mentioned, the setter was noted as being deliberately left out of the AMQP bits when that method was added). I do not think we should emit group-sequence values that were never set on the message.
   
    I'm done giving feedback on Artemis issues for a while.


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

[GitHub] activemq-artemis issue #2418: ARTEMIS-2142 Fix ServerJMSMessage

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

    https://github.com/apache/activemq-artemis/pull/2418
 
    So i looked at this some more to see what i can do, i simply can't work around the fact the OpenWireMessage will always return an int, as such it will always set sequence, so where the producer is OpenWire then when we transform to any other protocol then it will exist.
   
    "      
    coreMessage.putIntProperty(AMQ_MSG_GROUP_SEQUENCE, messageSend.getGroupSequence());
    "
    So we will need to simply handle mapping this better during converters.
   
    I can though remove the setters i had added into AMQPMessage, i think this is the most controversial part. Ill update with those bits removed.


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

[GitHub] activemq-artemis issue #2418: ARTEMIS-2142 Fix ServerJMSMessage

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

    https://github.com/apache/activemq-artemis/pull/2418
 
    Pushed update and build with above mentioned changes,


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

[GitHub] activemq-artemis issue #2418: ARTEMIS-2142 Fix ServerJMSMessage

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

    https://github.com/apache/activemq-artemis/pull/2418
 
    Just a note, not related to fixing the current sequence issue.
   
    But if we left the setters in they would fix a regression in feature around enforced broker side user, that was caused with the native AMQP message support.


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

[GitHub] activemq-artemis issue #2418: ARTEMIS-2142 Fix ServerJMSMessage - Fix curren...

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

    https://github.com/apache/activemq-artemis/pull/2418
 
    @gemmellr your suggestions are great, i haven't ignored your comments, im still working on small fix, it is a good idea you suggested to have this seperated as 2 commits/prs. Just working on how still. please bear with me. Im hoping i have something soon.


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

[GitHub] activemq-artemis issue #2418: ARTEMIS-2142 Fix ServerJMSMessage - Fix curren...

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

    https://github.com/apache/activemq-artemis/pull/2418
 
    @gemmellr im closing this, ive split this into two now as you had suggested.
   
    First is the minimal changes needed to fix the immediate issue https://github.com/apache/activemq-artemis/pull/2424
   
    Second is the refactor thats a more complete fixup around this area and cleaner (but larger change)  https://github.com/apache/activemq-artemis/pull/2425
   
    This way the first PR can be cherry picked if needed to HF branches as you suggested.
   
    Im closing this one now, with the above to replace it.


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

[GitHub] activemq-artemis pull request #2418: ARTEMIS-2142 Fix ServerJMSMessage - Fix...

jbertram-2
In reply to this post by jbertram-2
Github user michaelandrepearce closed the pull request at:

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


---