Closed embano1 closed 4 years ago
I apologise for my comment about using the PR template. It seems that we forgot to add it to the repo.
Please can you reformat this PR description? The most important part is this question
How has this been tested?
https://github.com/openfaas/faas/blob/master/.github/PULL_REQUEST_TEMPLATE.md
Thanks for the PR
and 3 second UpstreamTimeout for invoking functions to
not block too long.
This seems a bit too short. The default for functions in the watchdog is 10s, I would suggest maybe having that to match the default?
What do you think to using the async invocation mode in the connector-sdk, I'm pretty sure that was added at some point?
How did you decide on 5 second resync period, what if this was made configurable instead?
Add a helm chart maybe? I covered creating these in my latest tutorial/lab - https://github.com/alexellis/helm3-expressjs-tutorial
Thx @alexellis for your quick review! Updated the PR with the template and will that one going forward!
This seems a bit too short. The default for functions in the watchdog is 10s, I would suggest maybe having that to match the default?
No prob, will reset and update the docs accordingly.
How did you decide on 5 second resync period, what if this was made configurable instead?
1s (default) seemed rather short (was afraid of overwhelming the API if everyone follows this). But I'm totally fine reverting to 1s if this is deemed appropriate. Either way, I'll update the docs accordingly.
What do you think to using the async invocation mode in the connector-sdk
Sounds reasonable. Any drawbacks of this approach vs sync (besides the "immediate" feedback?)
Created https://github.com/openfaas-incubator/connector-sdk/issues/45 to discuss intervals/timeouts in the SDK.
1s (default) seemed rather short (was afraid of overwhelming the API if everyone follows this). But I'm totally fine reverting to 1s if this is deemed appropriate. Either way, I'll update the docs accordingly.
Wow was it really 1 second? That is not a good choice in retrospect. So I'm totally fine with bumping it higher, thank you for the suggestion.
Sounds reasonable. Any drawbacks of this approach vs sync (besides the "immediate" feedback?)
In your scenario where you may be running for 10s per invocation the async approach would fire and forget, so keep up with the system emitting events (vcenter), but without it, it could potentially fail if many events arrived at once.
I'm seeing different values to the one you mentioned?
Seems like we had 15s upstream to match the default watchdog setting + some grace, then topic map rebuilds every 10s?
Seems like we had 15s upstream to match the default watchdog setting + some grace, then topic map rebuilds every 10s?
Thx, I was checking the connector-sdk for examples and the only one provided was for the tester. Let me revert.
In your scenario where you may be running for 10s per invocation the async approach would fire and forget, so keep up with the system emitting events (vcenter), but without it, it could potentially fail if many events arrived at once.
That was also a concern I had, especially when using interpreted runtimes (PowerShell, PowerCLI). The drawback is I don't see function responses (just err on invocation or 202 Accepted
, which for now seems to be fine.
Running final test with the recent changes and will update this comment accordingly. Do you want me to squash the commits before merging?
[Update] Tests passed and updated the first comment accordingly.
Given that the commits add unwanted behaviour then revert it, I think it'd be better to squash that out of the history completely. :+1:
I just want to leave a final comment here on async invocation. I think async is the way to go for this connector to not limit throughput. However, I came across this issue https://github.com/openfaas/faas/issues/1298 where I think the author is correct and this behavior could cause issues especially when troubleshooting.
The only information the connector receives upon function invocation is basically the err status of the underlying http call (http.Client.Do()
). If the connection to the gateway is intact but the triggered function is not available (anymore [1]) and this change has not been synced to the topic map the response would still be "202" signaling accept (which per REST standard is not useful at all during troubleshooting). What's the recommended way to ease troubleshooting with connectors using async invocation?
[1] E.g. (to be) deleted during rolling upgrade or not accepting new connections
Thx @alexellis
Approved, but please make the async invocation an optional 12-factor configuration via environment variable.
Good point. I can file a follow-up commit making async optional via -async
flag. Right now the connector mainly uses flag for configuration so I think it'd be a good candidate for a flag.
Given that sync
makes it easier to troubleshoot and debug function invocations, especially for new users, I'll default -async
to false
and update the docs accordingly.
Let me know if this works and I'll create a PR.
Description
The connector now leverages the latest connector-sdk features such as allowing multiple event topic subscriptions (delimited with ",") and printing invokation responses via the
controller.Subscribe
interface (implemented byevents.NewEventReceiver()
.The controller uses a 5 second
RebuildInterval
(sync function subscriptions) and 3 secondUpstreamTimeout
for invoking functions to not block too long.Function comments are line-wrapped. Updates to Gopkg.toml to use the latest releases for imported packages:
Fixes #33
Motivation and Context
design/approved
labelHow Has This Been Tested?
[1]
Output of
vcenter-connector
:Output of
of-echo
:Types of changes
Checklist:
git commit -s
Signed-off-by: Michael Gasch mgasch@vmware.com