Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARTEMIS-2437 Allow extended types in annotations in AMQP to Core #2795

Closed
wants to merge 1 commit into from

Conversation

tabish121
Copy link
Contributor

When converting from AMQP to core and back again support annotations that
aren't able to be placed into Core message properties by storing the bytes
from encoding the types to AMQP encodings and then decoding them again
when converting back into AMQP messages.

@michaelandrepearce
Copy link
Contributor

Im a bit lost why this is needed. I thought if amqp in and amqp out we did no conversion

@tabish121
Copy link
Contributor Author

Large messages or messages that travel across a cluster are still converted

@michaelandrepearce
Copy link
Contributor

Rather than this, would it not be better address those then to avoid the conversion. I know clebert wanted at one time to make amqp large message support a required feature for 2.8.0 obv that didnt happen but maybe doing that and addressing the other bit maybe better than adding further workarounds that will need to be maintained

@tabish121
Copy link
Contributor Author

As reported in the original JIRA when an AMQP message is converted to core that contains complex AMQP types inside annotation sections the converter currently blows up instead of handling the message which is not correct as a core client should still be able to receive the message. For the cases of conversions due to large messages or moving over clusters the current broker again cannot handle those and so this is a reasonable fix as it preserves the AMQP data. If you want to contribute a complete solution such that AMQP to AMQP the message is never converted that would be welcome however until that time a fix is needed.

@michaelandrepearce
Copy link
Contributor

michaelandrepearce commented Aug 14, 2019

So i checked, the bridging code (e.g. intra cluster etc) encodes the message natively then embeds inside a core, as such the original message is fully preserved as is.
This just then leaves two issue areas:

  1. Large Messages
  2. Conversion to Core

Currently core doesn't support complex types, as such i think its wrong to leach AMQP encoding into core, as people could start relying on that. Either we should support complex types in core properly or we don't support and those are dropped. Reason for this is what occurs when other protocols support complex types we will be left with a legacy of AMQP encoded inside core.

Re large messages this just sounds like we simply aren't fully supporting AMQP large messages there are a number of issues i understand in this area, thus why Clebert wanted to make Native large message support a blocker ticket for 2.8 (shame this didnt happen), this is known, either we need to support them or we don't.

As such i see a few better options here:

  1. Support Large Messages for AMQP fully
  2. Support complex types in CORE
  3. Support 1 & 2

Im against having a solution that leaks AMQP encoding into CORE. just as much as i know we dont like to modify AMQP in transit. I think this should be a no go area.

@michaelandrepearce
Copy link
Contributor

Just to be clear this is not a negative vote but more im trying to invoke a discussion as there will be an element of once done will be harder to undo if people start relying on it for amqp to core. hoping that others will chime in like @clebertsuconic who was wanting to make amqp large message support native a blocker for a next release anyhow.

@gemmellr
Copy link
Member

I think this is a reasonable enough thing to do in the circumstance. The properties that would be created are already fairly AMQP interop centric through their names, as the existing related ones already are, so the content being especially so when required doesnt seem particularly odd. Dropping them might work if another way was currently found to preserve them in flight for AMQP recipients, but would be a bit unexpected that some things transit while others silently dont.

Rewiring the brokers large message support doesnt seem like a small thing to do, I wouldnt tie this comparatively small issue to that personally. Doing so still wouldnt address the cross-protocol aspect where this issue was actually hit and reported originally.

While complex types are usable in others areas such as the annotations section as seen here, AMQP does not permit complex application-properties, so extending Core for that would actually introduce [more?] scope for the same problem in reverse for Core to AMQP and the other protocols, so I dont think I would go there to resolve this either. Even if doing so, any user custom described types would probably still require something along these lines I think (or again, dropping).

When converting from AMQP to core and back again support annotations that
aren't able to be placed into Core message properties by storing the bytes
from encoding the types to AMQP encodings and then decoding them again
when converting back into AMQP messages.

Requires update to proton-j 0.33.2 for encoding fix
@clebertsuconic
Copy link
Contributor

clebertsuconic commented Aug 19, 2019

@tabish121 can you rebase please?

(edit: Never mind: some git-fu and I rebased it myself)

if you could check after merged please.

@asfgit asfgit closed this in cfdec52 Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants