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.

Sign up for GitHub

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

Jump to bottom

Added AnyFilterBenchmark and AllFilterBenchmark #3454

Merged
merged 3 commits into from
Nov 15, 2023
Merged

Added AnyFilterBenchmark and AllFilterBenchmark #3454

merged 3 commits into from
Nov 15, 2023

Conversation

MeenuyD
Copy link
Contributor

@MeenuyD MeenuyD commented Nov 1, 2023

feat: Add AnyFilterBenchmark and AllFilterBenchmark to benchmark filter list

Description:In this pr have added support for running benchmarks for AnyFilterBenchmark and AllFilterBenchmark by updating the filter-class-list.txt file. Now, users can run these benchmarks using the command ./hack/run.sh benchmark-filter AnyFilterBenchmark and ./hack/run.sh benchmark-filter AllFilterBenchmark. This enhancement enables performance testing for AnyFilter and AllFilter, providing valuable insights into their efficiency.

Persona:
This feature is essential for developers and performance testers who are working on optimizing the filter functionality within the system. By including AnyFilterBenchmark and AllFilterBenchmark in the benchmark filter list, they can assess the performance impact of changes made to these filters.

@knative-prow knative-prow bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 1, 2023
@knative-prow Knative Prow
Copy link

knative-prow bot commented Nov 1, 2023

Hi @MeenuyD. Thanks for your PR.

I'm waiting for a knative-extensions member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@codecov Codecov
Copy link

codecov bot commented Nov 1, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8a79f31) 61.58% compared to head (bb132c3) 61.58%.
Report is 10 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff            @@
##               main    #3454   +/-   ##
=========================================
  Coverage     61.58%   61.58%           
  Complexity      762      762           
=========================================
  Files           181      181           
  Lines         12405    12405           
  Branches        265      265           
=========================================
  Hits           7640     7640           
  Misses         4163     4163           
  Partials        602      602
Flag Coverage Δ
java-unittests 70.88% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Cali0707
Cali0707 reviewed Nov 2, 2023
View reviewed changes
DEVELOPMENT.md Outdated
Comment on lines 184 to 194
To run benchmarks for any filter benchmark , you can use:

```shell
./hack/run.sh benchmark-filter AnyFilterBenchmark
```

Alternatively, if you want to run all of the benchmarks you can run:

```shell
./hack/run.sh benchmark-filters
./hack/run.sh benchmark-filter AllFilterBenchmark
```
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this change makes sense, as we are already pointing users to the list of available filters on line 177. The lines you deleted were actually showing that you can call ./hack/run.sh benchmark-filters, which will benchmark all of the filters, not just one of them.

Would you be able to revert the changes to this file?

@knative-prow knative-prow bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 2, 2023
Cali0707
Cali0707 reviewed Nov 2, 2023
View reviewed changes
DEVELOPMENT.md Outdated Show resolved Hide resolved
@MeenuyD @Cali0707
Update DEVELOPMENT.md
bb132c3 
Co-authored-by: Calum Murray <[email protected]>
Cali0707
Cali0707 reviewed Nov 13, 2023
View reviewed changes
Copy link
Member

@Cali0707 Cali0707 left a comment

Choose a reason for hiding this comment

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

/lgtm
/cc @creydr

@knative-prow knative-prow bot requested a review from creydr November 13, 2023 14:31
@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Nov 13, 2023
@Cali0707
Copy link
Member

/test upgrade-tests

@Cali0707
Copy link
Member

/retest-required

@Cali0707
Copy link
Member

/ok-to-test
/retest-required

@knative-prow knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 15, 2023
@Cali0707
Copy link
Member

/cc @pierDipi

@knative-prow knative-prow bot requested a review from pierDipi November 15, 2023 16:15
pierDipi
pierDipi approved these changes Nov 15, 2023
View reviewed changes
Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

/approve

@knative-prow Knative Prow
Copy link

knative-prow bot commented Nov 15, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MeenuyD, pierDipi

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 Nov 15, 2023
@pierDipi
Copy link
Member

/test upgrade-tests

@Cali0707
Copy link
Member

/retest-required

1 similar comment
@Leo6Leo
Copy link
Contributor

Leo6Leo commented Nov 15, 2023

/retest-required

@knative-prow knative-prow bot merged commit b06df7b into knative-extensions:main Nov 15, 2023
18 checks passed
@Cali0707 Cali0707 mentioned this pull request Nov 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cali0707 Cali0707 Cali0707 left review comments

@pierDipi pierDipi pierDipi approved these changes

@Leo6Leo Leo6Leo Awaiting requested review from Leo6Leo

@odacremolbap odacremolbap Awaiting requested review from odacremolbap

@creydr creydr Awaiting requested review from creydr

Assignees

@pierDipi pierDipi

@Cali0707 Cali0707

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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MeenuyD @Cali0707 @pierDipi @Leo6Leo