Skip to content

Conversation

@cursedquail
Copy link
Contributor

The schema definition for the DescribeProducersRequest see here has trailing tokens - specifically, the last two lines in the commit in that link.

This does not cause problems for the generator, because Jackson will ignore trailing input by default.

However, some JSON parsers cannot be configured to ignore trailing characters, and so they fail on that file. This can cause problems for users wishing to use the official schema definitions to generate clients in other languages.

The fix here is pretty simple - just remove the trailing tokens, and optionally configure jackson to fail on trailing tokens. This patch is for the former, and I'll be happy to submit a patch that configures jackson so this won't happen again :).

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

{ "name": "PartitionIndexes", "type": "[]int32", "versions": "0+",
"about": "The indexes of the partitions to list producers for." }
]}
]}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Could we re-indent the Topics (L24) block now that we have removed the trailing json? We usually two spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1e9d680 🎉

@dongjinleekr
Copy link
Contributor

Nice Catch. 👍👍👍

@cursedquail cursedquail requested a review from dajac May 17, 2021 13:35
Copy link
Member

@dajac dajac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the patch.

@dajac
Copy link
Member

dajac commented May 17, 2021

@NLincoln Regarding your suggesting to configure Jackson to not ignore trailing tokens, that might be a good idea. Could you file a separate JIRA for it?

@cursedquail
Copy link
Contributor Author

@dajac done! https://issues.apache.org/jira/browse/KAFKA-12800

I can't edit the assignee field of that ticket, but I can submit a patch for it tonight :)

@dajac
Copy link
Member

dajac commented May 17, 2021

@dajac done! https://issues.apache.org/jira/browse/KAFKA-12800

I can't edit the assignee field of that ticket, but I can submit a patch for it tonight :)

Awesome, thanks.

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.

3 participants