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
Remove broken exec
build target, replace with something better.
#12307
Conversation
Currently this target is "yet another way" to run elasticsearch, which we can't maintain. It also has the problem that it doesnt ensure its running on the latest source code, doesn't configure any scratch space properly, won't work with securitymanager, list goes on. Even if we made it work, it would break every day, since its untested. Instead, `mvn package -Drun -DskipTests` will run packaging, and then startup bin/elasticsearch (like integration tests, but in foreground). It also enables debugger socket on port 8000, for people that like IDE debuggers and not system.out.println. Its a little slower to get started because of all the shading/RPM/DEB building going on in `package` but that is just what it is right now until that stuff is moved out.
LGTM, two questions:
Regardless, thanks for fixing the exec target, I will use this all the time! |
Its hooked into compilation. You got something else going on...
not easily. currently this shares the integration test directory. I am getting really worried here honestly. I think if you want to run 3 nodes you should not use targets like this but run elasticsearch properly. We gotta keep it simple... |
LGTM |
I am hostaging this pull request until we get consensus on the multiple nodes stuff. This is just not a direction i think we should take. |
My +1 is totally not contingent on either of the questions. I'm +1 to push as-is. I understand it's not something we should support, I just wondered if it was a byproduct. |
This is reproducible for me: git checkout c2c8956
mvn clean compile test-compile
git checkout pr/12307
mvn package -Drun -DskipTests Then - http://p.draines.com/1437144342682a61b674a.txt (there's nothing special about c2c8956, I just picked an older commit from a week ago). I guess I expect Maven to see that files have been changed and compile/clean accordingly? Is this an incorrect expectation? (I don't know Maven very well at all) Again, not anything that should prevent this PR from being merged, I'm just trying to understand Maven's behavior. |
This pr depends on so that problem you have, I mean it is a problem, but its unrelated to this PR and an existing problem in The .zip file mvn package builds is broken in some cases if the workspace is dirty. If we were to release that, all users would get that exception. Lets open a side issue for that one. It means that maven package isn't working well if the workspace is dirty. |
Sounds good, I'll open an issue for this. |
The issue might be in Maven itself? https://cwiki.apache.org/confluence/display/MAVEN/Incremental+Builds |
I think its something we should not try to do, until the integration tests support multiple nodes and have jobs running in jenkins that do that. Indeed this is something we probably need, to hook in backwards compatibility tests nicely and other things. But we don't have it today. The main point of this PR is to give a target that doesn't break every day for peoples IDEs/dev environments. The whole trick here to achieve this is to lean on stuff that is tested by jenkins. I think this is really important and if we start doing fancy stuff just for the dev environment only, then we are right back where we are today, no better than having the current broken So I'd rather tackle the tests first... and in general with all of this, we need to walk before we can run. |
That sounds very reasonable, thanks for the explanation! |
Remove broken `exec` build target, replace with something better.
Currently this target is "yet another way" to run elasticsearch, which we can't maintain since its an untested configuration. Besides the fact that even if we fixed it, it would break all the time since its untested, It also has the problem that it doesnt ensure its running on the latest source code, doesn't configure any scratch space properly, won't work with securitymanager, list goes on.
Instead,
mvn package -Drun -DskipTests
will run packaging, and then startup bin/elasticsearch(.bat) .. like integration tests, but in foreground).It also enables debugger socket on port 8000, for people that like IDE debuggers and not system.out.println.
Its a little slower to execute because of all the shading/RPM/DEB building going on in
package
but that is just what it is right now until that stuff is moved out.Logic is kind of in a wierd place, but thats also the point: its using our integration test logic that is in jenkins, so it is less likely to break.
Looks like this: