You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by clebertsuconic <gi...@git.apache.org> on 2017/02/20 17:00:22 UTC
[GitHub] qpid-proton-j pull request #6: PROTON-1410 Moving ThreadLocal logic for Deco...
GitHub user clebertsuconic opened a pull request:
https://github.com/apache/qpid-proton-j/pull/6
PROTON-1410 Moving ThreadLocal logic for Decoders on Message to its own class
I need to reuse the same Decoder used by the MessageImpl when creating my own special Message Type.
https://issues.apache.org/jira/browse/PROTON-1410
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/clebertsuconic/qpid-proton-j proton-1410
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/qpid-proton-j/pull/6.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 #6
----
commit 32eff8b6fe8db32e13c4c95ef5fb72c5166bdd98
Author: Clebert Suconic <cl...@apache.org>
Date: 2017-02-20T07:50:16Z
PROTON-1410 Moving ThreadLocal logic for Decoders on Message to its own class
I need to reuse the same Decoder used by the MessageImpl when creating my own special Message Type.
https://issues.apache.org/jira/browse/PROTON-1410
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j pull request #6: PROTON-1410 Moving ThreadLocal logic for Deco...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic closed the pull request at:
https://github.com/apache/qpid-proton-j/pull/6
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j issue #6: PROTON-1410 Moving ThreadLocal logic for Decoders on...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/qpid-proton-j/pull/6
@gemmellr I am doing the same thing... avoiding using MessageImpl from Proton.
I just feel weird having to do a copy & paste inheritance every time someone needs that functionality and it would be better on some utility class to be reused.
It's just an utility class, I can rename to any name you like.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j issue #6: PROTON-1410 Moving ThreadLocal logic for Decoders on...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/qpid-proton-j/pull/6
```a third bit of code stikes me as ugly,```
- as I said the best I envision is not not need multiple instances. Meanwhile what do you suggest? duplicating the code and the instances as we are doing now?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j issue #6: PROTON-1410 Moving ThreadLocal logic for Decoders on...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the issue:
https://github.com/apache/qpid-proton-j/pull/6
I'm aware of what qpid-jms does, it does that while avoiding using the Message[Impl] from proton entirely, meaning it isn't using the thread local from there either, but just happens to have its own (though it could actually do the same without that thread local if it wanted, given how the rest of the client works.) I wouldnt be all that inclined to make qpid-jms use the proposed TLSEncoder class even if it already existed in proton, it seems nicer to keep that in the client.
As an aside, TLS seems a weird acronym to use in the name, given its typical meaning/usage.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j issue #6: PROTON-1410 Moving ThreadLocal logic for Decoders on...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/qpid-proton-j/pull/6
Both qpid-jms and artemis are holding this type of TLS somewhere else.
I am creating my own version of an object that is only holding the byte[] of the message, and I only need it to decode and re-encode headers and application properties.
- https://github.com/apache/qpid-jms/blob/master/qpid-jms-client/src/main/java/org/apache/qpid/jms/provider/amqp/message/AmqpCodec.java#L61-L76
- https://github.com/apache/activemq-artemis/blob/master/artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/message/JMSMappingOutboundTransformer.java#L109-L124
My final proposal would be to have it singleton, but meanwhile I still need that functionality.. I think it would be better to be reusing the same TLS that's already used inside ProtonJMS instead of duplicating the variable.
I am expanding the usage to a new AMQPMessage on my branch, and it's using a delegate approach where I will try to not load the Message, and only parse the header and eventually applicationProperties:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j issue #6: PROTON-1410 Moving ThreadLocal logic for Decoders on...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the issue:
https://github.com/apache/qpid-proton-j/pull/6
Can you elaborate a little on what you are doing to need this? Are you just subclassing the MessageImpl? If so would protected getter(s) work for you?
Creating a separate new 'non impl' utility class purely to access a thread local of a specific implementation from within a third bit of code stikes me as ugly, especially if it is already a subclass of the starting point.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j issue #6: PROTON-1410 Moving ThreadLocal logic for Decoders on...
Posted by tabish121 <gi...@git.apache.org>.
Github user tabish121 commented on the issue:
https://github.com/apache/qpid-proton-j/pull/6
Personally this doesn't seem a necessary addition to me. I prefer to keep the management of the encoder and decoder instances in the Qpid JMS code and wouldn't use this myself. It adds support code that then has to be maintained as part of the proton-j code that just means we have to manage it later if the Encoders were say made to not require being managed as thread locals for instance, e.g. we made them stateless
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j issue #6: PROTON-1410 Moving ThreadLocal logic for Decoders on...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/qpid-proton-j/pull/6
This supersedes #5
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j issue #6: PROTON-1410 Moving ThreadLocal logic for Decoders on...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/qpid-proton-j/pull/6
I can live without this one.
The other with the delivery size would be needed for me. I will rework that one and close this one.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org