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

Add benchmark filter run script #3271

Merged

Conversation

Cali0707
Copy link
Member

@Cali0707 Cali0707 commented Aug 4, 2023

Fixes #3270

Proposed Changes

  • Add run.sh script in the benchmark project to run the benchmarks
  • Make the run.sh script runnable from hack/run.sh
  • Add documentation to DEVELOPMENT.md on how to run the script

@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/data-plane labels Aug 4, 2023
@Cali0707
Copy link
Member Author

Cali0707 commented Aug 4, 2023

/cc @matzew @creydr @Leo6Leo

@knative-prow knative-prow bot requested review from creydr and Leo6Leo August 4, 2023 18:29
@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Merging #3271 (b262332) into main (24ef5f8) will decrease coverage by 0.07%.
Report is 13 commits behind head on main.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #3271      +/-   ##
============================================
- Coverage     61.89%   61.82%   -0.07%     
- Complexity      765      766       +1     
============================================
  Files           181      181              
  Lines         12200    12220      +20     
  Branches        268      266       -2     
============================================
+ Hits           7551     7555       +4     
- Misses         4061     4078      +17     
+ Partials        588      587       -1     
Flag Coverage Δ
java-unittests 71.67% <ø> (-0.35%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 8 files with indirect coverage changes

@Cali0707
Copy link
Member Author

Cali0707 commented Aug 8, 2023

/cc @Leo6Leo

@knative-prow knative-prow bot requested a review from Leo6Leo August 8, 2023 15:31
@Cali0707
Copy link
Member Author

Cali0707 commented Aug 8, 2023

/retest-required

1 similar comment
@Cali0707
Copy link
Member Author

Cali0707 commented Aug 8, 2023

/retest-required

Copy link
Contributor

@Leo6Leo Leo6Leo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@Cali0707 the file is really large lol, and it is still running

@Cali0707
Copy link
Member Author

Cali0707 commented Aug 8, 2023

image

@Cali0707 the file is really large lol, and it is still running

This should be fixed now!

Copy link
Contributor

@Leo6Leo Leo6Leo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running benchmarks for ExactFilterBenchmarkjjkyj
No matching benchmarks. Miss-spelled regexp?
Use EXTRA verbose mode to debug the pattern matching.
Successfully ran benchmarks for ExactFilterBenchmarkjjkyj!\n\nThe results can be found at /home/leoli/Downloads/eventing-kafka-broker/data-plane/benchmarks/output/ExactFilterBenchmarkjjkyj.1691602228.out.txt

When the input is wrong, the error is this. And it will run the build first as well. If the className sanity check happens at the beginning, it can save some time and resources

@Cali0707
Copy link
Member Author

/cc @Leo6Leo

@knative-prow knative-prow bot requested a review from Leo6Leo August 10, 2023 20:07
@Leo6Leo
Copy link
Contributor

Leo6Leo commented Aug 14, 2023

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2023
DEVELOPMENT.md Outdated Show resolved Hide resolved
@knative-prow knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 15, 2023
@Cali0707
Copy link
Member Author

/cc @matzew

@knative-prow knative-prow bot requested a review from matzew August 15, 2023 14:17
@Cali0707
Copy link
Member Author

/retest-required

Comment on lines +40 to +45
while IFS="" read -r p || [[ -n "$p" ]]; do
if [[ "$1" == "$p" ]]; then
FOUND=1
break
fi
done <"${SCRIPT_DIR}/resources/filter-class-list.txt"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not an issue, but more a question: why didn't you use something like egrep -q "^${1}$"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just didn't think of it as I had already written the loop to go through the lines of filter-class-list.txt to run the benchmarks for all of the classes 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@creydr do you think I should switch? Your solution does seem cleaner to me

Copy link
Contributor

@creydr creydr Aug 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatever you prefer. I am fine with both

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think i'll leave it for now since it keeps the script more consistent with itself

@Cali0707 Cali0707 requested a review from creydr August 15, 2023 15:33
@creydr
Copy link
Contributor

creydr commented Aug 16, 2023

/retest

@creydr
Copy link
Contributor

creydr commented Aug 16, 2023

/lgtm
/approve

/hold
Feel free to unhold, when you're OK with #3271 (comment)

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 16, 2023
@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 16, 2023
@knative-prow
Copy link

knative-prow bot commented Aug 16, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, creydr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 16, 2023
@Cali0707
Copy link
Member Author

/unhold

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 16, 2023
@Cali0707
Copy link
Member Author

/retest-required

@knative-prow knative-prow bot merged commit c402ae9 into knative-extensions:main Aug 16, 2023
16 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/data-plane lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Event Filtering: Add hack/run script for java filter benchmarks
4 participants