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

Proposal: change the responce code in eventshub receiver for dropped events #211

Closed
xinydev opened this issue Jun 3, 2021 · 7 comments · Fixed by #216
Closed

Proposal: change the responce code in eventshub receiver for dropped events #211

xinydev opened this issue Jun 3, 2021 · 7 comments · Fixed by #216
Assignees

Comments

@xinydev
Copy link
Contributor

xinydev commented Jun 3, 2021

I am writing an e2e test for apiServerSource's retry feature, but the cloudevents.Client only retries when the HTTP response code is one of these.

I want to use eventshub to simulate a events lost scenario but the eventshub returns http.StatusConflict when dropping events. and the http.StatusConflict is not one of the codes which cloudevents.Client retries.

So I have a question. Is there any special reason to choose http.StatusConflict ?

Can we change it to 429(Too Many Requests) or 503(Service Unavailable) or 504(Gateway Timeout) for some tests scenario like retring?

Or is there any advices for this e2e test?

@xinydev
Copy link
Contributor Author

xinydev commented Jun 4, 2021

@vaikas What do you think of this?

@vaikas
Copy link
Contributor

vaikas commented Jun 4, 2021

Not totally sure if this would fit your use case, but for RabbitMQ Broker, I ended up using this:
https://github.com/knative-sandbox/eventing-rabbitmq/blob/main/pkg/dispatcher/dispatcher.go#L80

With some more context here:
cloudevents/sdk-go#596

@xinydev
Copy link
Contributor Author

xinydev commented Jun 4, 2021

Not totally sure if this would fit your use case, but for RabbitMQ Broker, I ended up using this:
https://github.com/knative-sandbox/eventing-rabbitmq/blob/main/pkg/dispatcher/dispatcher.go#L80

With some more context here:
cloudevents/sdk-go#596

Thanks! It is very helpful!

@xinydev xinydev closed this as completed Jun 4, 2021
@vaikas
Copy link
Contributor

vaikas commented Jun 4, 2021

Great to hear :) Ping me if you need some more help :)

@xinydev
Copy link
Contributor Author

xinydev commented Jun 15, 2021

@vaikas Hi, what do you think we add a new env DropEventsResponseCode to config the status code returned when dropping events ?

I think in my scenario(knative/eventing#5457), it might not be good to modify the default strategy of cloudevent.Client
in pkg/adapter for the convenience of writing tests, because It may affect other components that use cloudevent.Client.

@xinydev xinydev reopened this Jun 15, 2021
@pierDipi
Copy link
Member

pierDipi commented Jun 16, 2021

@xinydev makes sense to me, and it will be a lot useful for testing the behavior of some specific status codes with the upcoming changes here https://github.com/knative/specs/pull/24/files#diff-e71710278fc6ee3296a229b7448808ab0869b6ce9b3232a695b6f57fbc7ea6fdR95

@xinydev
Copy link
Contributor Author

xinydev commented Jun 16, 2021

/assign

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.

3 participants