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

fix: updated topic configuration object data types #242

Merged

Conversation

gokerakc
Copy link
Contributor

@gokerakc gokerakc commented Feb 13, 2024

Description

The following properties need to be updated as long:

  • retention.ms

  • retention.bytes

  • delete.retention.ms

If you need further details about the configs you can check this documentation page: https://kafka.apache.org/documentation/#topicconfigs_retention.bytes

Change log

  • Updated data types from integer to long

@gokerakc gokerakc force-pushed the fix_UpdateTopicConfigurationDataTypes branch 2 times, most recently from e0cab70 to 194986a Compare February 13, 2024 14:42
Copy link
Collaborator

@dalelane dalelane left a comment

Choose a reason for hiding this comment

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

I'm absolutely not a schema expert here, but I thought we're speaking the language of JSON schema here, which means we're restricted to "integer" as a way of describing whole numbers.

e.g. https://json-schema.org/understanding-json-schema/reference/numeric

Happy for you to explain why I'm wrong though :)

@gokerakc
Copy link
Contributor Author

If we are dependent on JSON in this README.md file then you are right, we can not use long as a data type.

But also it would be good to raise that developers should use long in their tooling otherwise they may end up having problems.

@dalelane
Copy link
Collaborator

That's a fair point - I'm perhaps being unhelpfully strict for a markdown file, and this is better thought of as an issue for https://github.com/asyncapi/spec-json-schemas only, where you do have them as integers.

@smoya - what do you think?

@smoya
Copy link
Member

smoya commented Feb 20, 2024

That's a fair point - I'm perhaps being unhelpfully strict for a markdown file, and this is better thought of as an issue for https://github.com/asyncapi/spec-json-schemas only, where you do have them as integers.

@smoya - what do you think?

The issue is that you can't specify long as a type in JSON Schema, so we won't be able to really ensure it.
Anyway, I also agree this README should mention long to be more accurate.

@dalelane
Copy link
Collaborator

fair enough - this way round makes sense then - the formal json schema is only as precise as it can be (integer) but we give a more helpful/accurate description in the markdown doc here

Thanks, both, for hearing me out :)

@gokerakc gokerakc force-pushed the fix_UpdateTopicConfigurationDataTypes branch from 194986a to f83867d Compare February 20, 2024 15:16
@gokerakc gokerakc force-pushed the fix_UpdateTopicConfigurationDataTypes branch from f83867d to 963cbe6 Compare February 20, 2024 15:28
@gokerakc
Copy link
Contributor Author

/rtm

@asyncapi-bot asyncapi-bot merged commit 581aab1 into asyncapi:master Feb 20, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants