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

Wrong offset records sent definition in the OpenAPI #789

Closed
ppatierno opened this issue Apr 23, 2023 · 3 comments · Fixed by #790
Closed

Wrong offset records sent definition in the OpenAPI #789

ppatierno opened this issue Apr 23, 2023 · 3 comments · Fixed by #790

Comments

@ppatierno
Copy link
Member

When an HTTP client sends more records in one HTTP request, it could happen that one of the record cannot be sent for any reasons (i.e. not existing partition, ...).
For example, let's consider the following HTTP producer request body:

{
    "records": [
        {
            "key": "key-1",
            "value": "value-1",
        },
        {
            "value": "value-2",
            "partition": 1
        },
        {
            "key": "key-3",
            "value": "value-3"
        }
    ]
}

Where the topic doesn't have partition 1, but just partition 0.
The second record send will fail and the bridge returns the following body in the HTTP response:

{
    "offsets": [
        {
            "partition": 0,
            "offset": 30
        },
        {
            "error_code": 404,
            "message": "Topic my-topic not present in metadata after 60000 ms."
        },
        {
            "partition": 0,
            "offset": 31
        }
    ]
}

The second element is not a partition/offset pair but the error about why the second record was not sent.

Today, this behavior is not reflected in the OpenAPI where the "offesets" array is declared as an array of OffsetRecordSent while it could also be a Error

"OffsetRecordSentList": {
                "title": "OffsetRecordSentList",
                "type": "object",
                "properties": {
                    "offsets": {
                        "type": "array",
                        "items": {
                            "$ref": "#/components/schemas/OffsetRecordSent"
                        }
                    }
                },

It should be something like:

"offsets": {
        "type": "array",
        "items": {
        	"oneOf": [
                            {
                                "$ref": "#/components/schemas/OffsetRecordSent"
                            },
                            {
                                "$ref": "#/components/schemas/Error"
                            }
                        ],
            
        }
    }
@ppatierno
Copy link
Member Author

ppatierno commented Apr 23, 2023

The only concern I have about this is backward compatibility.
I mean ... if users are using the current OpenApi specification to auto-generate an HTTP client, the proposed change will change such a generation.
It's also true that a client generated by the current OpenApi is wrong, because in case of error on sending a specific record, the client would not be able to handle it because not defined by the spec and not expected.
A correct client would be generated by making the proposed change.
@strimzi/maintainers wdyt?

@ppatierno
Copy link
Member Author

@tombentley @scholzj may I get your opinion on this change please? Thanks!

@scholzj
Copy link
Member

scholzj commented Apr 24, 2023

I guess I would say we should fix the OpenAPI spec. But TBH, this is not really my area of expertise, so not sure how it might impact the users.

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 a pull request may close this issue.

2 participants