-
Notifications
You must be signed in to change notification settings - Fork 58
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
Support for prefix/equals-ignore-case and suffix/equals-ignore-case #121
Conversation
* Adding functionality for prefix/equals-ignore-case and suffix/equals-ignore-case with the respective unit tests, benchmarks, and README updates.
* Explicitly running the benchmarks class after mvn verify is executed. By default, the maven-surefire-plugin only runs tests with the "Test" which ends up ignoring the Benchmarks test class.
After adding the "Run benchmarks" step on Github actions, we started seeing failures on Ubuntu for JDK8 due to GC overhead limits. This commit changes the command to only run CL2 benchmarks.
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, just few documentation updates would be good to merge along with this change.
@@ -588,12 +646,24 @@ public void CL2Benchmark() throws Exception { | |||
|
|||
bm = new Benchmarker(); | |||
|
|||
bm.addRules(PREFIX_EQUALS_IGNORE_CASE_RULES, PREFIX_EQUALS_IGNORE_CASE_MATCHES); | |||
bm.run(citylots2); | |||
System.out.println("PREFIX_EQUALS_IGNORE_CASE_RULES events/sec: " + String.format("%.1f", bm.getEPS())); |
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.
update this section https://github.com/aws/event-ruler/blob/main/README.md#performance ?
@@ -45,10 +45,18 @@ public static ValuePatterns prefixMatch(final String prefix) { | |||
return new ValuePatterns(MatchType.PREFIX, prefix); | |||
} | |||
|
|||
public static ValuePatterns prefixEqualsIgnoreCaseMatch(final String prefix) { |
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.
this part of the doc needs an update https://github.com/aws/event-ruler/blob/main/README.md#the-patterns-api
@@ -29,3 +29,5 @@ jobs: | |||
cache: 'maven' | |||
- name: Verify with Maven | |||
run: mvn --batch-mode --errors --update-snapshots verify | |||
- name: Run benchmarks |
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.
you can also update the comment within this template, so folks know how to update fetch the benchmark results (for now just pasting JDK 8 results is fine)
https://github.com/aws/event-ruler/blob/main/.github/PULL_REQUEST_TEMPLATE.md
* Updated "Performance" section on README with benchmarks results * Updated "THe Patterns API" section on README with the new match types * Updated PR template with instructions on where to get the benchmarks
Issue #, if available: #108
Description of changes:
Benchmark / Performance (for source code changes):
From my mac laptop (2.6 GHz 6-Core Intel Core i7 / 16 GB 2667 MHz DDR4):
From the Github benchmark:
Java 8:
Java 11:
Java 17:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.