Skip to content

Remove benchmark package #15356

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

Merged
merged 1 commit into from
Dec 11, 2015
Merged

Remove benchmark package #15356

merged 1 commit into from
Dec 11, 2015

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented Dec 10, 2015

Tons of ancient "benchmarks" exist in elasticsearch. These are main
methods that do some kind of construction of ES classes and time various
things. The problem with these is they are not maintained, and not run.
Refactorings that touch anything that is common in these classes is very
painful. Going through these, almost all would simply not work in 2.x
without modifications (because they do not set path.home).

This change removes the entire benchmark package. If someone needs to
run a benchmark like this, they can look at history for examples if
necessary (although these examples are often not realistic and should
just start real elasticsearch processes in a shell script). Longer term,
we should make this easier to do by having the build support adding real
benchmarks which can be run in jenkins (so we know they actually run,
instead of doing refactorings with pure guesswork as to whether the
benchmark would run correctly).

@jpountz
Copy link
Contributor

jpountz commented Dec 10, 2015

+1 but I know this can be controversial so I would suggest to wait a bit before merging

@jasontedor
Copy link
Member

I've pondered the same thing myself many times during some refactoring efforts; I'm in favor of this.

LGTM.

@kimchy
Copy link
Member

kimchy commented Dec 10, 2015

I am +1 on this change. What I would ask is that we capture the intent of some of these benchmarks into a meta issue in in our perf testing infra to make sure we end up having coverage for those and testing them in a continuous manner. Things like indexing perf for json doc with "just" string and a number, many aliases tests, ... (/cc @danielmitterdorfer)

@danielmitterdorfer
Copy link
Member

Funny, I have also came across the benchmark classes and felt the same way. So +1 too. I'll create a meta issue over in the Rally repo and reference this ticket so we don't forget it. Thanks for bringing it to my attention @kimchy.

@jpountz
Copy link
Contributor

jpountz commented Dec 10, 2015

I would suggest to wait a bit before merging

OK let's merge now :)

Tons of ancient "benchmarks" exist in elasticsearch. These are main
methods that do some kind of construction of ES classes and time various
things. The problem with these is they are not maintained, and not run.
Refactorings that touch anything that is common in these classes is very
painful. Going through these, almost all would simply not work in 2.x
without modifications (because they do not set path.home).

This change removes the entire benchmark package. If someone needs to
run a benchmark like this, they can look at history for examples if
necessary (although these examples are often not realistic and should
just start real elasticsearch processes in a shell script). Longer term,
we should make this easier to do by having the build support adding real
benchmarks which can be run in jenkins (so we know they actually run,
instead of doing refactorings with pure guesswork as to whether the
benchmark would run correctly).
rjernst added a commit that referenced this pull request Dec 11, 2015
@rjernst rjernst merged commit 7f4ef9f into elastic:master Dec 11, 2015
@rjernst rjernst deleted the clean_benchmarks branch December 11, 2015 03:30
rjernst added a commit that referenced this pull request Dec 11, 2015
@ywelsch
Copy link
Contributor

ywelsch commented Mar 15, 2016

@danielmitterdorfer I have removed two more benchmark classes in d14ae5f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants