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

use spaces liberally in integration tests and fix space handling #12710

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
8 participants
@rmuir
Copy link
Contributor

rmuir commented Aug 7, 2015

By using pathnames with spaces in tests we can kickout all the bugs.
I applied the fix for #12709 but we needed more fixes actually.

TODO: windows

@rjernst

This comment has been minimized.

Copy link
Member

rjernst commented Aug 7, 2015

LGTM

@rmuir

This comment has been minimized.

Copy link
Contributor Author

rmuir commented Aug 7, 2015

Everything in the root of our ES dir has a space, so a few parameters for pidfile, repo path, classpath, etc have spaces. ES home itself has a space, and cwd (which is different) has a space. bin/plugin is also fed urls with spaces in the plugin smoketester.

I removed all of ant argline usage (except what is fed by jenkins, which is gc params etc) to reduce the possibility of bugs, it makes the testing clear.

TODO: fix any bat files for windows. Test the ES_GC_LOG_FILE by just setting one up in integ tests. I will look into these tomorrow.

@jaymode

This comment has been minimized.

Copy link
Member

jaymode commented Aug 7, 2015

+1 thank you for doing this

@nik9000

This comment has been minimized.

Copy link
Contributor

nik9000 commented Aug 7, 2015

LGTM

@rmuir

This comment has been minimized.

Copy link
Contributor Author

rmuir commented Aug 7, 2015

See my comment, i am not sure this is really 100% correct still. try something like this:

diff --git a/dev-tools/src/main/resources/ant/integration-tests.xml b/dev-tools/src/main/resources/ant/integration-tests.xml
index f67701b..2b45b05 100644
--- a/dev-tools/src/main/resources/ant/integration-tests.xml
+++ b/dev-tools/src/main/resources/ant/integration-tests.xml
@@ -140,6 +140,7 @@
       <attribute name="es.http.port" default="${integ.http.port}"/>
       <attribute name="es.transport.tcp.port" default="${integ.transport.port}"/>
       <attribute name="es.pidfile" default="${integ.pidfile}"/>
+      <attribute name="es.gc.logfile" default="${integ.scratch}/gc@{es.http.port}.log"/>
       <attribute name="jvm.args" default="${tests.jvm.argline}"/>
     <sequential>

@@ -152,6 +153,7 @@
           <env key="JAVA_HOME" value="${java.home}"/>
           <!-- we pass these as gc options, even if they arent, to avoid conflicting gc options -->
           <env key="ES_GC_OPTS" value="@{jvm.args}"/>
+          <env key="ES_GC_LOG_FILE" value="@{es.gc.logfile}"/>
           <arg value="-Des.cluster.name=@{es.cluster.name}"/>
           <arg value="-Des.http.port=@{es.http.port}"/>
           <arg value="-Des.transport.tcp.port=@{es.transport.tcp.port}"/>

Problem is that JAVA_OPTS and ES_JAVA_OPTS are not handled correctly and all folded in one string i think, and need something like the previous 'eval' reparsing that was happening (but ONLY for those two, not the rest of the commandline, and hopefully cleaner).

I am going to go for a long run today and try to catch up on some errands. If someone who actually knows bash wants to take this from me, i would be more than happy. I dont know shell, i just hack until mvn verify passes. And there is still windows to possibly fix :(

But I think we want to fix this soonish, so argument processing is really correct.

@rmuir

This comment has been minimized.

Copy link
Contributor Author

rmuir commented Aug 7, 2015

and the fastest way to iterate here is to change files then do ./run.sh which uses all of this logic, but runs in foreground and is easy to debug.

@uschindler

This comment has been minimized.

Copy link
Contributor

uschindler commented Aug 7, 2015

The usual horror :) it's so simple with ant...

please fix!

@rmuir

This comment has been minimized.

Copy link
Contributor Author

rmuir commented Aug 7, 2015

Also the way ES here accepts -D arguments, parses them as es parameters only to call System.setProperty on each one, that's totally bogus. These need to go to the JVM, or it causes very difficult-to-debug issues, like if you set -Djava.io.tmpdir=..., its not really happening until too late, and by then JVM has already initialized e.g. temp file helpers with the default tmpdir in clinits, so your system property does not have the effect you like.

@rmuir

This comment has been minimized.

Copy link
Contributor Author

rmuir commented Aug 7, 2015

for the last part, just change Bootstrap parser to require es prefix on all those -D's and fail with an exception otherwise. Then there is no confusion.

@rmuir

This comment has been minimized.

Copy link
Contributor Author

rmuir commented Aug 7, 2015

The worst is leniency, $ bin/elasticsearch -Doops.a.typo=4 does nothing but silently work. These settings need to be in a static list somewhere. plugins can register their own lists (could be managed as e.g. properties file, maybe needs wildcard support) and we fail if its an unknown setting.

@jasontedor

This comment has been minimized.

Copy link
Member

jasontedor commented Aug 7, 2015

@rmuir There are related issues for that: #6732, and #12657 (the latter being closed in favor of the former).

Setting an unknown setting, or a setting that can't be changed should throw an error.

@rmuir

This comment has been minimized.

Copy link
Contributor Author

rmuir commented Aug 7, 2015

That issue is full of confusion and nonsense. It should be done in a simple way with e.g. properties file for checks (and plugins can have them too). each property (prolly needs wildcard/pattern in some case from what i see) should include its type as well (if its being accessed by getAsBoolean its a boolean, etc), and a short description. This also means its easy to do really nice stuff like list all supported properties, "did you mean" in error messages, etc later.

@clintongormley

This comment has been minimized.

Copy link
Member

clintongormley commented Aug 10, 2015

Even with this PR, wildcards are still problematic:

bin/elasticsearch --http.cors.enabled=true --http.cors.allow-origin='*'  -d

Fails with:

ERROR: Parameter [-d]does not start with --

If the -d comes before the wildcard, then it works

@spinscale

This comment has been minimized.

Copy link
Member

spinscale commented Aug 10, 2015

@clintongormley looks like a parser issue (independent from this PR), when regular arguments come after the --foo.bar style args.. investigating

s1monw added a commit to s1monw/elasticsearch that referenced this pull request Aug 11, 2015

Improve arg handling in ant script
This commit is a spinn-off from elastic#12710 which improves the handling of process
arguments ot play nice with spaces etc.

s1monw added a commit that referenced this pull request Aug 11, 2015

Improve arg handling in ant script
This commit is a spinn-off from #12710 which improves the handling of process
arguments ot play nice with spaces etc.

@clintongormley clintongormley referenced this pull request Aug 11, 2015

Closed

Testing and packaging changes for 2.0 #12768

16 of 16 tasks complete

@rmuir rmuir closed this in 0f61f56 Aug 11, 2015

@s1monw s1monw deleted the rmuir:spaces_galore branch Aug 11, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.