Fix issues with JMX configuration #164
Conversation
| @@ -82,7 +82,7 @@ const ( | |||
| // Create a JMX Configuration map to convert values from CR to how they look like as env vars | |||
| var JMXConfigurationMap = map[string]string{ | |||
| "JMXRemote": "-Dcom.sun.management.jmxremote=", | |||
| "JMXRemotePort": "-Dcom.sun.management.jmxremote.port=", | |||
| "JMXRemotePort": "-Dcom.sun.management.jmxremote.rmi.port=", | |||
There was a problem hiding this comment.
why change it to rmi, can you point to some documentation that shows it's required ?
There was a problem hiding this comment.
From cassandra-env.sh file:
else
JVM_OPTS="$JVM_OPTS -Dcassandra.jmx.remote.port=$JMX_PORT"if ssl is enabled the same port cannot be used for both jmx and rmi so either
pick another value for this property or comment out to use a random port (though see CASSANDRA-7087 for origins)
JVM_OPTS="$JVM_OPTS -Dcom.sun.management.jmxremote.rmi.port=$JMX_PORT"
I didn't have a connection if I specified only jmxremote.port. It worked for me only with jmxremote.rmi.port
There was a problem hiding this comment.
I'll update the patch where we specify both parameters with the same port.
api/v2/cassandracluster_types.go
Outdated
| JMXRemotePort string `json:"jmxRemotePort,omitempty"` | ||
| // JXMRemoteSSL defines is SSL for JMX connections enabled or not | ||
| JXMRemoteSSL string `json:"jmxRemoteSSL,omitempty"` |
There was a problem hiding this comment.
Why change those types ? why can't you make it work with the current types which make more sense ?
There was a problem hiding this comment.
I agree with you, but with boolean types and with using omitempty when we specify these params as false they will be omitted. If we remove omitempty it will have default value as false.
We can use pointers here, but I thought that string values would be a more convenient way.
There was a problem hiding this comment.
Yes I think it makes more sense to keep it strongly typed and use pointers, thanks !
* use pointers for bool fields to prevent omitting "false" values. * add jmx RMI port parameter
|
I've updated patch, new e2e tests run - https://github.com/gzimin/casskop/actions/runs/13030664236 |
|
thank you @gzimin for your contributions ! |
What's in this PR?
Fixes for previous pull-request with new JXM parameters.
Why?
Previous patch has some issues
Additional context
Checklist