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

YAMLSelector bugfix for Knative RPS #621

Merged
merged 1 commit into from
Mar 26, 2025

Conversation

cvetkovic
Copy link
Contributor

@cvetkovic cvetkovic commented Mar 25, 2025

Removing redundant fields that added consistency problems.

Signed-off-by: Lazar Cvetković <l.cvetkovic.997@gmail.com>
@cvetkovic cvetkovic requested a review from leokondrashov March 25, 2025 12:53
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.

Looks fine to me right now.

However, I would argue that we should make it possible to create Azure trace parser and rps generator without passing knative-specific and/or dirigent-specific arguments. At least, we should extract parsing the trace/generating the function inv data from the filling all of the necessary fields in the Function structure.

@cvetkovic
Copy link
Contributor Author

I wanted to remove the YAMLSelector from the Function specification, but vSwarm traces rely on this since functions do not share the YAML file, therefore I left it.

@cvetkovic cvetkovic merged commit 46b208f into main Mar 26, 2025
15 checks passed
@cvetkovic cvetkovic deleted the cvetkovic/bugfixing-knative-rps-yaml-selector branch March 26, 2025 17:52
@leokondrashov
Copy link
Contributor

My point was that we can generate functions with all invocations first and then fill the rest of the fields in a separate method. That would let us generate the invocations without requiring any system-specific parameters/arguments since it is system-agnostic. The function structure doesn't need to be changed; we can leave the fields as nils during the invocation generation phase.

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.

2 participants