-
Notifications
You must be signed in to change notification settings - Fork 18
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
"Creating a simple DAG Driver for Knative" #383
Conversation
Please elaborate more on the operation of the proposed changes, why, what, how in the PR description. The documentation should also be in the repo docs. Also, follow instructions from vhive contributing guide and signoff your commits. |
Have updated the PR description, loader.md document and signed off. Do let me know if there is anything else to update. |
Please be more descriptive of how it should be used. Like, you didn't mention the structure of the new file with DAG structure. Moreover, there is no example file. This is important since not everyone would dive into the code to determine the file's structure. In addition, fix the spellcheck CI (add the word into the wordlist |
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.
Please remove duplicate code. Also, the line
Added new structs InvocationMetadataDAG to parse in the list of functions into individualDAGDriver()
in the description confused me, that we have some file that has the description of the DAG, while we just invoke all of them one after another. Please fix the wording.
690202a
to
713825e
Compare
f46fdc9
to
452614d
Compare
LGTM. @cvetkovic Please review 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.
Can you write a unit test for a trace driver with DAG mode enabled?
8c3b383
to
13e0ac7
Compare
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.
LGTM! Thanks for the added test
Signed-off-by: Kway Yi Shen <kwayyishen@gmail.com>
Summary
In current implementation, single execution of
go run cmd/loader.go --config cmd/config.json
would create anindividualFunctionDriver
per function and invoke individual functions with a set frequency.The new implementation would create a new
functionsDriver
which invoke all the functions sequentially, following the invocation frequency of the first function.Implementation Notes ⚒️
DAGMode
incmd/config.json
to toggle DAGMode.functionsDriver()
in the full list of provided functions and invokes them sequentially in a loop.Node
structure to hold function pointers and pointer to the next NodeinvokeGRPC()
to reflect invocation stats for each function.External Dependencies 🍀
Breaking API Changes⚠️
Simply specify none (N/A) if not applicable.