> > I noticed in DatabaseDescriptor there are setters that are leaking it to > JMX since cassandra-3.0. I am not sure whether we can just change it to > IllegalArgumentException which will be then also thrown on startup where > they were ConfigurationExceptions before
My concern here is maintenance, you can fix it once but it will break again. I feel we need a more general solution, open to input here! Looking into our own NodeTool we have two error codes - 1 that catches a > few java and airlift exceptions and 2 for other errors. My guess is we aim > to throw only exceptions that will lead to exit code 1, no? I am not sure. Good point, nodetool is also a client to the JMX, but fully knows our API, whatever solution we do we need to be careful with nodetool's assumptions. Looking at org.apache.cassandra.tools.NodeTool#execute it doesn't look like these errors are JMX related, but nodetool related (airlift is the CLI parser used in nodetool); but IllegalArgumentException and IllegalStateException could have been caused from a JMX call (RuntimeException thrown from Cassandra would result in status = 2) In many setters we were actually not doing the same checks we do on Startup > too… I consider this being a bug. Agree, if you can bypass our guards via JMX, that sounds like a bug to me On Wed, Apr 6, 2022 at 8:50 AM Ekaterina Dimitrova <e.dimitr...@gmail.com> wrote: > I would say keep the old ones and add new ones to be compatible in > addition. > Now your catch led me to more thinking and I think throwing > ConfigurationException deserves more attention. I noticed in > DatabaseDescriptor there are setters that are leaking it to JMX since > cassandra-3.0. I am not sure whether we can just change it to > IllegalArgumentException which will be then also thrown on startup where > they were ConfigurationExceptions before. > Not sure what is the right course of action here to fix things without > being considered breaking changes too as I see different people doing > different things in our codebase. > Looking into our own NodeTool we have two error codes - 1 that catches a > few java and airlift exceptions and 2 for other errors. My guess is we aim > to throw only exceptions that will lead to exit code 1, no? I am not sure. > GetColumnIndexSize was added in trunk and I see it throws > ConfigurationException, that was acknowledged in its test. Considering the > setter was always doing it I guess this was just normal addition but It > seems to me it is actually a bug… Now I can change it to throw > IllegalArgumentException. Looking at the code I think different people > probably have different understanding so I want to align us to ensure we > don’t break tools by fixing or not stuff… > In many setters we were actually not doing the same checks we do on > Startup too… I consider this being a bug. > > On Tue, 5 Apr 2022 at 18:21, David Capwell <dcapw...@gmail.com> wrote: > >> There are 2 places that expose non-standard java classes, so JMX only >> works if and only if the JMX client also has Cassandra's jars, else >> they will fail; the 2 examples are >> >> org.apache.cassandra.service.StorageServiceMBean#enableAuditLog throws >> org.apache.cassandra.exceptions.ConfigurationException, removing that >> exception does not break binary compatibility, but does break source >> as javac will say catching it isn't allowed as it doesn't throw. If >> you call the method without Cassandra jars the method will work >> properly until a ConfigurationException is thrown, in which case a >> ClassNotFoundException will be thrown, hiding the error message. >> >> >> org.apache.cassandra.db.ColumnFamilyStoreMBean#forceCompactionForTokenRange >> takes a collection of >> org.apache.cassandra.dht.Range<org.apache.cassandra.dht.Token>, since >> this is an operation the only people who could call it are those with >> the jars in the path. >> >> Given that both are not properties and require setting values, jmx get >> object does not break, so the impact is limited to the callers of each >> method. >> >> We have a few options to address: >> >> 1) live with it. Allow these cases to keep doing what they are doing, >> but block new cases from popping up >> 2) live with it, but expose new versions which do not break JMX >> 3) remove ConfigurationException from >> StorageServiceMBean#enableAuditLog and accept the source compatibility >> issue >> 4) remove or fix ColumnFamilyStoreMBean#forceCompactionForTokenRange >> 5) #3 and #4 >> >> What do others think? In CASSANDRA-17527 I am adding a test to detect >> these cases and block them, I am also fixing the JMX issues on trunk >> that were not released, so this thread is only limited to the cases >> that were actually released >> >> Thanks! >> >
