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

[Feature Request] Include contextual information in responses #26

Closed
bmcustodio opened this issue Aug 7, 2019 · 4 comments
Closed

[Feature Request] Include contextual information in responses #26

bmcustodio opened this issue Aug 7, 2019 · 4 comments

Comments

@bmcustodio
Copy link
Contributor

bmcustodio commented Aug 7, 2019

Currently, a ResponseSubscriber gets no contextual information whatsoever about a given request in its Response method. This means that if, for example, I am writing a connector for a given message broker, I cannot mark a given message as having been processed / requiring a retry based on the response(s) because I have no way of correlating responses with whatever message/... originated them.

As a (hopefully) more concrete example, we are building a connector for AWS SQS queue that receives messages from a queue and invokes a function with each message's payload, and we need to...

  1. Delete the message if the function could process it successfully.
  2. Change its visibility timeout if the function could not process it successfully.

I thought about two ways of working around this, both of which require code changes.

  1. Support specifying a string-valued correlationID parameter which would be passed to Invoke and be echoed back in each response.
  2. Support specifying a context which would work the same way, with the added benefit of being able to carry multiple values (and not just a single correlation ID) whenever that's required, as well as to be used as the actual HTTP requests' context.

I chose to go with solution (2) as #25 because it seems to be the most flexible one to me.

@alexellis
Copy link
Member

It sounds like you want a way to "ack" that a message has been received or processed?

Could you do that in the code that dequeues a message and invokes the function?

For the Kafka Connector, we invoke, followed by marking the offset:

https://github.com/openfaas-incubator/kafka-connector/blob/master/main.go#L99

https://github.com/openfaas-incubator/kafka-connector/blob/master/main.go#L101

What if you did the same in your SQS connector?

Is the shortfall that you only want to ack messages conditionally depending on whether the function returned a 2xx status code?

@alexellis
Copy link
Member

Something which may make this harder to model is that each "Invoke()" call, may lead to an invocation of 0..* functions:

https://github.com/openfaas-incubator/connector-sdk/blob/master/types/invoker.go#L40

What happens if a message is executed by 1 function successfully and a second unsuccessfully? What if no functions exist and there are no status codes?

Alex

@bmcustodio
Copy link
Contributor Author

What if you did the same in your SQS connector?

I looked into that but that doesn't cut it for us.

Is the shortfall that you only want to ack messages conditionally depending on whether the function returned a 2xx status code?

Kind of, for now. In the future we may need to extend this behaviour, so we need a way of associating responses with the originating entity (message/request/...).

What happens if a message is executed by 1 function successfully and a second unsuccessfully? What if no functions exist and there are no status codes?

That would be up to whoever implements the connector to decide, provided we give them a way to understand if the invocations were successful (which we currently don't, and which my PR tries to introduce).

@alexellis
Copy link
Member

@bmcstdio do we have this now? Can I close?

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

No branches or pull requests

2 participants