-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
fixing run.bat and run.sh #14451
fixing run.bat and run.sh #14451
Conversation
def runEsHome="${project.buildDir}/run-elastic" | ||
mainClassName = "org.elasticsearch.bootstrap.Elasticsearch" | ||
applicationDefaultJvmArgs = ["-Xms256m", "-Xmx1g", "-Djava.awt.headless=true", "-Delasticsearch", "-Des.foreground=yes", | ||
"-ea", "-Des.path.home=" + runEsHome, "-Des.security.manager.enabled=false", |
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.
run.sh
was good because it was "real". It run with the security manager and with bin/elasticsearch
. So much real. Sadly this sacrifices a lot.
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.
Interesting, good to know, let me play around with these options. I took them from someone's configuration in the build-tools.
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.
I think its hard to reproduce the previous behavior without fixing #14361 first, but afaik, @rjernst is working on that right now.
at a high level: thats' what the previous code did: invoke the "integration test setup" but passing "fork=false", passing different port numbers (the conventional 9200/9300), and passing some additional jvm args to enable the remote debugger.
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.
I've removed the test argument, but the security manager has to stay disabled unfortunately.
Another way would be take the fresh distribution, unpackage, run. But that sounds like an overkill at this stage.
At least the run scripts will start a node, it can be improved on later.
I think we should do this right, which means mimicking what we did before with maven. The framework is mostly already there, it just needs to be setup. This means creating a "run" task, which uses ClusterFormationTasks, and also stalls when the task is executed. Unfortunately, this won't work as nice until gradle fixes finalizedBy (https://discuss.gradle.org/t/run-finalizedby-when-task-is-interrupted-ctrl-c-continued-from-old-forum/12036), which means the cluster will remain alive until you run a follow up task... |
Note that the task should also not be under core, but at the root, or under distribution, since it will need to depend on the zip distribution. |
I was surprised it worked in core to be honest, I only put it there because I had classpath issues with the previous run task. And now gradle is cheating on my behalf and I didn't even notice - it's gone off to the distribution module. Is there any way to enforce which modules should depend on which?
|
It's not cheating. The cluster formation tasks add a dependency on the zip distribution. It's doing exactly what is expected. I just think it is confusing to have core depend on distribution (since it looks like a circular dep, even though it isn't in this case). So I would move the run task to either the root (my preference for ease of use) or distribution. |
I've moved it to distribution, since if I move it to the root it can't find the zip.
Is there something I can configure to make it work? - first time I'm working with gradle. |
It tells you exactly the problem: Cannot resolve ... because no repositories are defined. However, rather than adding repositories to the root project, I think defining it under distribution is fine. We can even add a shortcut I think from the root file:
|
rebasing to master, uhh, that generated a lot of commits on the pr. |
I think you just need to pull the latest master? Also, can you remove the run.sh and run.bat scripts? I don't think they are useful now (before they were saving from typing a complicated mvn command). |
@andrejserafim This looks good, can you sync up with master and I will merge? |
And done. |
@andrejserafim, sorry to ask one more thing. I hadn't realized how many small noisy commits there are here. Can you rebase on master so there is one commit with a clear commit message? |
ca70333
to
8a8c5b1
Compare
run.sh and run.bat were calling out to the old maven build system. This is no longer in place, so we've created new gradle tasks to start an elasticsearch node from the current codebase. fixed elastic#14423
@rjernst anything else? |
Add gradle run command to replace run.bat and run.sh
Thank you @andrejserafim!! |
fixed #14423
I'm a little new to gradle, so do double check I haven't missed something basic.