I’d agree that anywhere we’re calling `nodetoolResult` or `nodetool` in a test, it would be better to enable JMX and use it rather than the older mocks we set up to enable calling the mbeans directly. I don’t think enabling JMX by default is the right way to go mostly due to the added resources/time required to run the tests (it’s only a few seconds of additional startup/shutdown time, but when running lots of tests every second counts). Also, all other features are only enabled when requested, so making JMX on by default would require us to change the general pattern and have a `without` method to turn off a feature?
Better, I think, just to require it to be explicitly turned on and then have the methods that call into nodetool on Instance just throw a clear exception if jmx is disabled. Doug > On Aug 25, 2023, at 6:35 AM, Brandon Williams <dri...@gmail.com> wrote: > > I would prefer to have one standard way to do it, and given the > options I would prefer it be proper JMX instead of mocking. > > Kind Regards, > Brandon > > On Fri, Aug 25, 2023 at 4:20 AM Miklosovic, Stefan > <stefan.mikloso...@netapp.com> wrote: >> >> Hi list, >> >> I want to gather a feedback for this comment (1). >> >> Long story short, until JMX feature was introduced, we kind of hacked / >> mocked the calls to MBeans from IInstance, like this (2). If you notice, >> there is a lot of methods throwing UnsupportedOperationException because we >> had no proper JMX connection in place. That in turn means that tests which >> call nodetool commands which are using these MBeans / operations are not >> possible. >> >> The fix I made in CASSANDRA-18572 will use JMX feature and it will hook >> nodetool to a proper JMX connection where we are not mocking anything etc >> ... It will use same stuff as in production. >> >> However, this is happening only if one uses JMX feature. So all existing >> tests calling nodetool without this feature will still use it like it was. >> The patch I made takes care of both scenarios. >> >> My question is if we should not make JMX feature turned on by default. That >> way we might further simplify the code base and get rid of the hacks. >> >> Another possibility is to not turn it on by default but we would add JMX >> feature to each test which is using nodetool. That would also mean that any >> future test which will use nodetool will fail if it does not have JMX >> feature enabled. >> >> What would you like to see - dual solution (proper JMX connection if such >> feature is used as well as the legacy way) or only one solution with a >> proper JMX? (enabled by default or not). >> >> Regards >> >> (1) >> https://issues.apache.org/jira/browse/CASSANDRA-18572?focusedCommentId=17758920&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17758920 >> (2) >> https://github.com/apache/cassandra/blob/trunk/test/distributed/org/apache/cassandra/distributed/mock/nodetool/InternalNodeProbe.java
