-
Notifications
You must be signed in to change notification settings - Fork 405
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
Hot protocols improvements (for 662) #724
Conversation
I have just noticed that the |
As for the others, there's no change in HTTP and the advertised changes in JSON:
|
Type string `json:"type"` | ||
Deadline string `json:"deadline"` | ||
Body string `json:"body"` | ||
Protocol *CallRequestHTTP `json:"protocol"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, this has to be documented. As follow-up, it is necessary to open issues against FDKs to adopt new changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added documentation in function-format.md. I will file issues on fdks.
if incomingReq.Deadline != ci.Deadline().String() { | ||
t.Errorf("Request Deadline assertion mismatch: expected: %s, got %s", | ||
ci.Deadline(), incomingReq.Deadline) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add method verification as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added method and request URL testing too.
I fixed the
|
Overall LGTM. One major concern, JSON format before this change was meant to be close nearly-compatible to CNCF OpenEvent structure, but with this change, we are about to make a step aside. @treeder made JSON proto as it is right now, so i'd like to know if we really want to make that type of changes. If so, i recommend to do the following: make JSON protocol interface embeddable, so if we'd like to bring OpenEvent to Fn there would just one thing to adjust - actual data structure to serialize. |
Our json format won't match openevent, which is fine. Once the dust settles on that, we'll just have another format, like |
LGTM |
api/agent/protocol/factory.go
Outdated
// something meaningful. | ||
// This assumes StartedAt was set to something other than the default. | ||
// If that isn't set either, then how many things have gone wrong? | ||
// TODO: assert or panic in that case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 let's just panic. if our tests don't catch this, our tests need more tests
LGTM. the amendments seem like only additions really at the top level with removal from the header bucket, so it won't hard break really. |
Closes #662.
This change will require FDKs which handle the
json
function format to be updated.This change moves some Fn-specific fields around in the JSON protocol structure, thus improving the handling of Fn-specific fields that would otherwise end up in FN-prefixed headers. The rationale for this is in #662.
Fn-specific stuff (Call ID, request url, etc) that used to be stored in the headers of the
models.Call
is now instead added by the protocol dispatchers at the right time, so that the model reflects what the user has originally provided. The HTTP protocol dispatcher still creates Fn-prefixed headers, because that's the only way it can pass information to the function, but the JSON protocol can provide the information directly in JSON fields.