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

"Creating a simple DAG Driver for Knative" #383

Merged
merged 1 commit into from
Mar 24, 2024

Conversation

NotAnAddictz
Copy link
Contributor

@NotAnAddictz NotAnAddictz commented Feb 28, 2024

Summary

In current implementation, single execution of go run cmd/loader.go --config cmd/config.json would create an individualFunctionDriver 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 ⚒️

  • Added a new parameter DAGMode in cmd/config.json to toggle DAGMode.
  • When DAGMode = true, functionsDriver() in the full list of provided functions and invokes them sequentially in a loop.
  • Added a new Node structure to hold function pointers and pointer to the next Node
  • Edited parameter in invokeGRPC() to reflect invocation stats for each function.
    Example Diagram (Sequential Invocation)

External Dependencies 🍀

  • (N/A)

Breaking API Changes ⚠️

  • (N/A)

Simply specify none (N/A) if not applicable.

@leokondrashov
Copy link
Contributor

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.

@NotAnAddictz
Copy link
Contributor Author

Have updated the PR description, loader.md document and signed off. Do let me know if there is anything else to update.

@leokondrashov
Copy link
Contributor

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 .github/configs/wordlist.txt).

Copy link
Contributor

@leokondrashov leokondrashov left a 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.

@NotAnAddictz NotAnAddictz force-pushed the main branch 2 times, most recently from 690202a to 713825e Compare March 10, 2024 12:59
@NotAnAddictz NotAnAddictz force-pushed the main branch 2 times, most recently from f46fdc9 to 452614d Compare March 13, 2024 03:46
@leokondrashov
Copy link
Contributor

LGTM. @cvetkovic Please review as well.

Copy link
Contributor

@cvetkovic cvetkovic left a 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?

@NotAnAddictz NotAnAddictz force-pushed the main branch 2 times, most recently from 8c3b383 to 13e0ac7 Compare March 20, 2024 12:58
@NotAnAddictz NotAnAddictz requested a review from cvetkovic March 20, 2024 13:06
Copy link
Contributor

@leokondrashov leokondrashov left a 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>
@wanghanchengchn wanghanchengchn merged commit 31c635c into vhive-serverless:main Mar 24, 2024
13 checks passed
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 this pull request may close these issues.

4 participants