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

Address content-type compatibility between messages generated by different versions of SCSt apps #1106

Closed
olegz opened this issue Oct 19, 2017 · 9 comments
Assignees
Labels
bug
Milestone

Comments

@olegz
Copy link
Contributor

@olegz olegz commented Oct 19, 2017

Due to the recent content-type negotiation improvements we unintentionally introduced compatibility issue where pre-2.0 consumers may not be able to properly negotiate the content type of the message generated by 2.0+ producers.
This is due to the fact that pre-2.0 producers and consumers were relying on the original-content-type header which was removed in 2.0.
Another change that was introduced as part of this effort deals with registering additional flag (spring.cloud.stream.bindings.input.legacyContentTypeHeaderEnabled) and interceptor (LegacyContentTypeHeaderInterceptor) to allow 2.0+ consumers to successfully process pre-2.0 messages. This issue is that it makes consumer statically aware of the producer and its version and could result in more issues if such consumer receives messages from the producers of different versions.

Proposal:

  1. 2.0 producer should re-introduce the original-content-type header. While this header will be ignored by 2.0 consumers it will be used by pre-2.0 consumers as before. It should also generate one additional header such as version (e.g., version=2.x). This additional header will be used by 2.0+ consumers to ignore the re-introduced original-content-type header (more details below).
  2. 2.0 consumer should have a mechanism (may be through the already existing LegacyContentTypeHeaderInterceptor) to check for the version header before checking for the existence of the original-content-type header. If 'version' header is pre-2.0 or is not present (which is what's going to be in the case of pre-2.0 producers) then we know it's a legacy message and content-type negotiation should be done using the original-content-type header, otherwise such header will be ignored. Basically the version header will play the same role as newly introduced spring.cloud.stream.bindings.input.legacyContentTypeHeaderEnabled flag, but it will come as part of the message instead of static configuration of the consumer. Further more, making it a version instead of a boolean will give us more flexibility in the future when it comes to similar issues especially if we decide to drop support for certain version of messages.
@garyrussell

This comment has been minimized.

Copy link
Contributor

@garyrussell garyrussell commented Oct 20, 2017

What about Kafka? Does this imply that a 2.0 Kafka consumer has to interoperate with a 1.x producer (and vice-versa)?

We made the decision that 2.0 would only support >= Kafka 0.11; it sounds like this won't be possible now and we'll have to maintain support for the 0.10.x binder(s).

Of course, transactions and native headers won't be available if you use such a binder.

It will also be difficult to to eventually migrate to kafka 0.11 (or 1.0 when available) because it will need a big bang. To support a progressive migration, we'll need some work in the binder to not add native headers (so the 0.11 binder can talk to an 0.10 broker). When all services have migrated to 0.11 binder; they could then change out the broker; they would still then need a "big bang" to enable native headers across all the services.

Documenting all this is going to be difficult.

Sigh.

@sobychacko

This comment has been minimized.

Copy link
Contributor

@sobychacko sobychacko commented Oct 20, 2017

@garyrussell

This comment has been minimized.

Copy link
Contributor

@garyrussell garyrussell commented Oct 20, 2017

(since 1.3 can work with 0.11 by ignoring native headers and transactions)

But that's not the whole picture; when using 1.3 with the 0.11 binder it does add headers. If you use 1.3 with a 0.10 binder, it doesn't use headers.

2.0 will not be able to produce messages for a 1.3/0.10 consumer, unless we add code to skip headers and reinstate support for embedded headers.

@markpollack

This comment has been minimized.

Copy link
Contributor

@markpollack markpollack commented Oct 20, 2017

Was a design where a 2.0 producer didn't add this legacy field considered? The downside of this approach is dragging the legacy fields along which keeps potential confusion. While somewhat less comprehensive, an approach in which the latest 1.3.x release is made to be forward compatible, by being aware of where the new 2.0 fields/headers are located. I know it is complicated, but just wanted to present another approach in case it wasn't considered.

@olegz

This comment has been minimized.

Copy link
Contributor Author

@olegz olegz commented Oct 20, 2017

@markpollack that can work, just need to wrap my head around it. And it will require a new release of 1.3 pipeline (i.e., 1.3.1).
That said, Gary's Kafka points are something we need to take great caution, since it brings another variable into the picture (ability to support message headers natively). Here are few scenarios we should consider:

  1. Kafka 0.11 producer -> (0.11 broker) -> Kafka 0.10 consumer

  2. Kafka 0.10 producer -> (0.11 broker) -> Kafka 0.11 consumer
    . . . however in the 1 scenario the consumer won't be able to understand the message due native headers. In the 2 scenario, we can make consumer understand that headers are embedded in payload. However, there may/will be an issue with non-SCSt consumers for this scenarios as they may expect headers to be available natively and I am not sure we can do it with Kafka 0.10 client.

  3. Kafka 0.11 producer -> (0.10 broker) -> Kafka 0.10 consumer

  4. Kafka 0.10 producer -> (0.10 broker) -> Kafka 0.11 consumer
    The 3 may work if there is a way for the producer to know which broker version it's bound to, so it can determine how to deal with headers (embed or native). The 4 will work the same way as 2, however it will not have the non-SCSt client problem.

We obviously should not have problems when both producers, consumers and brokers are all of a same version and since 1.3 dropped support for pre 0.10 Kafka we don't have to worry about older brokers/clients.

@garyrussell

This comment has been minimized.

Copy link
Contributor

@garyrussell garyrussell commented Oct 20, 2017

I haven't (yet) found a way for the client to figure out the broker version (only the kafka-clients version).

In any case, even if we can figure out the broker version (say it's 0.11) the producer doesn't know whether the consumer is using the 0.11 client, so we can't make any assumptions based on the broker version anyway.

I have issued a PR adding a new HeaderMode (headers). This is a switch which will allow 2.0 producers/consumers to coexist with 1.x apps (albeit via configuration). The kafka binder will continue to default to embedded headers, thus easing migration. When all (associated) apps are 2.x, the user can reconfigure them to use the native headers; the 2.0 consumer should check for native headers and, if not present, see if we have embedded headers and, if so, extract them. We can potentially remove this fallback logic in 2.1 or 3.0.

I think this covers all the bases.

@olegz

This comment has been minimized.

Copy link
Contributor Author

@olegz olegz commented Oct 20, 2017

The only bases it is not covering is non-SCSt clients (producers/consumers), but as we discussed that level of the awareness may need to be delegated to such clients. Correct?

@garyrussell

This comment has been minimized.

Copy link
Contributor

@garyrussell garyrussell commented Oct 20, 2017

Most non-scst peers will be ok.

headerMode=none (P/C) when communicating with 0.10 peers (or 0.11 peers that don't populate or expect headers); no problems.

headerMode=headers (C) when communicating with 0.11 producers that populate headers - if the default header mapper detects the headers were'n't created by S-I-Kafka, it will return the raw headers; no problems.

headerMode=headers (P) when communicating with 0.11 consumers - a consumer expecting headers will either need to be made aware of the S-I-K header encoding, or the producer will need to be configured with a different header mapper implementation - we don't currently expose a mechanism for that, so we will need to.

@artembilan

This comment has been minimized.

Copy link
Contributor

@artembilan artembilan commented Oct 24, 2017

Reopen.
Closed automatically by merge relevant PR

@artembilan artembilan reopened this Oct 24, 2017
@olegz olegz added the in progress label Oct 25, 2017
sobychacko added a commit to sobychacko/spring-cloud-stream that referenced this issue Oct 25, 2017
- Remove binding config property `legacyContentTypeHeaderEnabled` that was
  introduced to enable legacy content type handling
- Enable LegacyContentTypeInterceptor in 2.0 always, but bypass any
  legacy content type handling if the message received is from
  a 2.0 producer by checking on the version header
- Introdce a new BinderHeader property for version
- Fix the LegacyContentType related tests
- Remove the check for originalContentType in ReceivingHandler when
  the payload received is a byte[]
- Remove unnecessary deserializePayloadIfNecessary calls in ReceivingHandler
- Remove deprecated deserializePayload methods in AbstractBinder
  and MessageSerializationUtils

Partly fixes spring-cloud#1106
Fixes spring-cloud#1110
@sobychacko sobychacko added in pr and removed in progress labels Oct 25, 2017
olegz added a commit to olegz/spring-cloud-stream that referenced this issue Oct 29, 2017
…ity issues

- This PR builds on the previous PR from Soby 6c259be (pr/1112)
- Added support in AbstractBinderTests to create bindable channel based on determining channel input type based on it's name (i.e., *input*)
- Added LegacyContentTypeHeaderInterceptor to TestSupportBinder to restor the previous behavior of MessageCollector for cases where Message's payload content type is a variant of 'text'.
- Restored tests that use MessageCollector to depend on proper payload type
- Added 'deserialize' routine back to MessageSerializationUtils
- Polished MessageConverterConfigurer.LegacyContentTypeHeaderInterceptor to remove conditional original-content-type header logic
- Removed default contentType from BindingProperties
sobychacko added a commit to sobychacko/spring-cloud-stream that referenced this issue Nov 7, 2017
…ity issues

Fixes spring-cloud#1106

- This PR builds on the previous PR with commit hash 6c259be... (pr/1112)
- Added support in AbstractBinderTests to create bindable channel based on determining channel input type based on it's name (i.e., *input*)
- Added LegacyContentTypeHeaderInterceptor to TestSupportBinder to restor the previous behavior of MessageCollector for cases
  where Message's payload content type is a variant of 'text'.
- Restored tests that use MessageCollector to depend on proper payload type
- Added 'deserialize' routine back to MessageSerializationUtils
- Polished MessageConverterConfigurer.LegacyContentTypeHeaderInterceptor to remove conditional original-content-type header logic
- Removed default contentType from BindingProperties
- Restored tests that use MessageCollector to depend on proper payload type
- polishing
- fixed Kryo/Java serialization
- Fixed ser/de for "application/json" and contentType equals
- Fixed of ser/de of JSON strings to ensure that Strings are not re-quoted
- Fixed how we comparing contentTypes
- more polishing
- Fixed NPE in MessageSerializationUtils
- Make bindings and consumer groups in new tests added to AbstractBinderTests mutually exclusive
@sobychacko sobychacko closed this in 6c259be Nov 7, 2017
@sobychacko sobychacko removed the in pr label Nov 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.