datastax / cstar_perf

Apache Cassandra performance testing platform
Apache License 2.0
72 stars 34 forks source link

GC stats no longer being collected #227

Closed tjake closed 8 years ago

tjake commented 8 years ago

I think before this change it happened to work since it was forcing the remote jmx server to start https://github.com/datastax/cstar_perf/commit/35340bd47fee4798b0fa5a25a360e8dfe8f6fed8

But since we get no stats from the servers since they are all listening locally. I can work around it by setting the following env variables.

LOCAL_JMX=no
JVM_EXTRA_OPTS="-Dcom.sun.management.jmxremote.authenticate=false"

If this could be added by default that would be great. Well you shouldn't set JVM_EXTRA_OPTS since thats needed by the user. But that flag needs to be set last to override the default JVM_OPTS

mambocab commented 8 years ago

Looks like JVM_OPTS has the necessary c.s.m.j.authenticate setting by default, if I read this correctly:

https://github.com/datastax/cstar_perf/blob/b429debfd843b34fd69ba475b6306589028906c2/tool/cstar_perf/tool/fab_common.py#L578

mambocab commented 8 years ago

But, you say you need to override JVM_EXTRA_OPTS -- why is that necessary? It looks like it's empty by default. This conditional is the only place it seems to be set, and it's not executed by default.

If I read this correctly, this should be fixed by setting LOCAL_JMX=no by default. Does setting that all by itself in the job config make GC stat collection work the way you want, @tjake?

This might all be moot though, because I'd like to talk about the possibility of reverting https://github.com/datastax/cstar_perf/commit/35340bd47fee4798b0fa5a25a360e8dfe8f6fed8. I don't understand the logic behind it. The changes mentioned in the commit message only affect 3.6, so why change defaults for running all versions @mshuler?

tjake commented 8 years ago

Because sometimes users want to set that. Since its applied last it overrides any pre-sets.

No setting LOCAL_JMX=no doesn't work by itself. I tried and it didn't work. I needed both

tjake commented 8 years ago

To be clear JVM_EXTRA_OPTS is used in cassandra-env.sh

mambocab commented 8 years ago

Because sometimes users want to set that. Since its applied last it overrides any pre-sets.

I think I understand what you're saying now, gotcha. So cstar_perf just ignores users' JVM_EXTRA_OPTS, huh?

@tjake What are your thoughts on reverting the commit that caused this? We may have to wait until next week to hear from Michael and make a decision on that.

mshuler commented 8 years ago

On 06/30/2016 10:44 AM, Jim Witschey wrote:

This might all be moot though, because I'd like to talk about the possibility of reverting 35340bd https://github.com/datastax/cstar_perf/commit/35340bd47fee4798b0fa5a25a360e8dfe8f6fed8. I don't understand the logic behind it. The changes mentioned in the commit message only affect 3.6, so why change defaults for running all versions @mshuler https://github.com/mshuler?

It has been a little time since I tested this out, but the setting was removed because:

a) it did nothing, since it was redundant to the default b) it logged a jmx bind failure on every pre-3.6 c* version I attempted c) a new c* commit to 3.6 started taking that logged jmx bind failure and made it a fatal startup error

Feel free to revert, although >= 3.6 c* will fail to start, unless some other better/more correct configuration mechanism is used such as Jake is suggesting.

Michael

tjake commented 8 years ago

I think we need to fix this by adding appending to JVM_EXTRA_OPTS

tjake commented 8 years ago

Like literally Add:

LOCAL_JMX=no JVM_EXTRA_OPTS="$JVM_EXTRA_OPTS -Dcom.sun.management.jmxremote.authenticate=false"

At the end of the passed ENV

mambocab commented 8 years ago

I can see why the LOCAL_JMX change is necessary, but I don't understand the JVM_EXTRA_OPTS part -- since JVM_EXTRA_OPTS just gets added to JVM_OPTS in cassandra-env, and c.s.m.j.authenticate is already set to false in fab_common, why is setting it in JVM_EXTRA_OPTS necessary?

tjake commented 8 years ago

Because later on its set to true which overrides your setting.

mambocab commented 8 years ago

hup, yeah, there it is:

https://github.com/apache/cassandra/blob/trunk/conf/cassandra-env.sh#L255

Ok, I see now. Making a PR now for Eduard/Michael's review