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

Remove allowExperimentalV011 flag #847

Merged
merged 10 commits into from
Sep 10, 2020
Merged

Conversation

Nevon
Copy link
Collaborator

@Nevon Nevon commented Aug 21, 2020

This has been defaulted to true for almost two years, so it's hardly experimental anymore.

src/protocol/requests/index.js is where the meat of the change is. Essentially if the flag was false, we'd filter out any api version above a certain limit for Fetch and Produce requests.

This has been defaulted to `true` for almost two years, so it's
hardly experimental anymore.
@Nevon Nevon requested review from tulios and JaapRood August 21, 2020 09:36
@Nevon
Copy link
Collaborator Author

Nevon commented Aug 21, 2020

Hmm, so this PR reveals a bit of nastiness. The tests that are currently failing are ones where allowExperimental011 was false. They now fail because we are returning some new data in the responses, which is normally fine except that some of that is from producer.send, which is a public API...

For example, a response when allowExperimental011 is false:

{
  "errorCode": 0,
  "offset": "0",
  "partition": 0,
  "timestamp": "-1",
  "topicName": "test-topic-aa740d312ca4c5c8d606-75297-2699790b-a3f0-4706-891c-7e7c2b410bc4",
}

And when it's true:

{
  "baseOffset": "0",
  "errorCode": 0,
  "logAppendTime": "-1",
  "logStartOffset": "0",
  "partition": 0,
  "topicName": "test-topic-aa740d312ca4c5c8d606-75297-2699790b-a3f0-4706-891c-7e7c2b410bc4",
}

Our type definitions say that the response the following, which is just a flat out lie for Kafka >=0.11:

type RecordMetadata = {
  topicName: string
  partition: number
  errorCode: number
  offset: string
  timestamp: string
}

Produce >=V3 has a different signature compared to V2, so anyone
using Kafka 0.11 or higher would have already had this signature at
runtime.
Enables running tests with a maximum Kafka version rather than a minimum version
These tests failed after enabling higher versions of Fetch, since they
assumed the 0.10 format. Changed the tests to only run in Kafka 0.10 and
verified that they pass.
Makes it clear if a test has LTE or GTE semantics.
@Nevon
Copy link
Collaborator Author

Nevon commented Aug 21, 2020

Updated the type definition of RecordMetadata to not be a lie anymore.

This also revealed a problem in how some of our tests were structured. Basically, any test that doesn't use a testIfKafka<version> function assumes that it will pass in any version of Kafka. For testing 0.11 or 1.1 features, we have testIfKafka_0_11 and testIfKafka_1_1_0. Now this assumption isn't right anymore, because we now have some tests that will pass in 0.10 but not in >0.10. So I had to create testIfKafka_0_10 - however, unlike the other ones, this one will only run if the Kafka version is less than or equal to 0.10. This felt weird, so I renamed them testIfKafka<AtMost|AtLeast>_<version>.

@ankon
Copy link
Contributor

ankon commented Aug 21, 2020

... RecordMetadata ... types ...

This somehow triggered something in my brain: You may have accidentally also fixed or improved #464 now :)

@Nevon
Copy link
Collaborator Author

Nevon commented Aug 21, 2020

Oh wow, I had completely forgotten about that. The new RecordMetadata type is now essentially an API version agnostic response type. However, I still think we should make that explicit at runtime (there's a convenient serializeResponses function where we could do it), but I'll leave that out of this PR, as it's already grown too much.

@tulios tulios merged commit ca6b68a into master Sep 10, 2020
@tulios tulios deleted the remove-experimental-011-flag branch September 10, 2020 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants