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

Set correct url to naming conventions. #477

Merged
merged 1 commit into from
Sep 14, 2022

Conversation

gklijs
Copy link
Contributor

@gklijs gklijs commented Sep 14, 2022

I noticed the url in the Java doc was not correct. As it turns out using a regex is about ten times slower for this case, so I removed that part of the commit.

@pierDipi
Copy link
Member

also think it would be preferable to use a regex for validating the extension name.

Would you mind expanding on the why?

@gklijs
Copy link
Contributor Author

gklijs commented Sep 14, 2022

It might be faster, although, with the expected small size of an extension name, that might not be the case. It does limit the lines of code, but the complexity of a regex might outweigh that.

@pierDipi
Copy link
Member

It might be faster, although, with the expected small size of an extension name, that might not be the case. It does limit the lines of code, but the complexity of a regex might outweigh that.

Exactly, a proper benchmark should be added to validate the change isn't a perf regression for various cases.

@gklijs gklijs force-pushed the use_pattern_for_extension_name branch from 4e83137 to 15e7ad2 Compare September 14, 2022 11:00
@gklijs
Copy link
Contributor Author

gklijs commented Sep 14, 2022

Thanks, I learned something today. Using the 3 extension names used in the unit test I quickly created a JMH test.

Result "io.cloudevents.bench.core.ValidExtensionNameBenchmark.validateUsingRegex":
  6191804.624 ±(99.9%) 144293.900 ops/s [Average]
  (min, avg, max) = (5863269.600, 6191804.624, 6608794.307), stdev = 192628.228
  CI (99.9%): [6047510.724, 6336098.524] (assumes normal distribution)
Result "io.cloudevents.bench.core.ValidExtensionNameBenchmark.validateUsingLoop":
  39559968.737 ±(99.9%) 1142943.753 ops/s [Average]
  (min, avg, max) = (35967212.664, 39559968.737, 40975142.182), stdev = 1525797.210
  CI (99.9%): [38417024.985, 40702912.490] (assumes normal distribution)

As the for loop implementation is almost 7 times faster, I reverted that part of the commit.

@gklijs gklijs changed the title Use pattern for validating extension names, and correct url to spec. Set correct url to naming conventions. Sep 14, 2022
@pierDipi
Copy link
Member

Hi @gklijs, thanks for the fix and the quick update, can you please fix the DCO check as explained here: https://github.com/cloudevents/sdk-java/pull/477/checks?check_run_id=8348631721?

Signed-off-by: Gerard Klijs <gerard.klijs@axoniq.io>
@gklijs gklijs force-pushed the use_pattern_for_extension_name branch from 15e7ad2 to daad91c Compare September 14, 2022 13:25
@gklijs
Copy link
Contributor Author

gklijs commented Sep 14, 2022

Should be good now.

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.

Thanks @gklijs!

LGTM

@pierDipi pierDipi merged commit adde53c into cloudevents:master Sep 14, 2022
@gklijs gklijs deleted the use_pattern_for_extension_name branch September 14, 2022 14:12
abutch3r pushed a commit to abutch3r/sdk-java that referenced this pull request Sep 22, 2022
Signed-off-by: Gerard Klijs <gerard.klijs@axoniq.io>
Signed-off-by: alex-butcher <21243172+abutch3r@users.noreply.github.com>
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