Filed CASSANDRA-17580 and have a patch ready which will hide non-native java exceptions from leaking into JMX (Cassandra and third-party libraries), this is enabled by default but can be disabled via config or system property
Config: jmx_hide_non_java_exceptions: false System property -Dcassandra.settings.jmx_hide_non_java_exceptions=false Does anyone see any issues about this? > On Apr 6, 2022, at 9:35 AM, David Capwell <dcapw...@gmail.com> wrote: > > 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!
