Please ignore my previous email, ZJeremiah pointed I didn’t see snakeyaml will show the name of the problem parameter, then in the constructor we provide the problem… that will be also in the trace. Seems I missed that :-( I will finish the extended classes approach. Excuse me for the noise, trying to do the right thing here…
On Fri, 22 Apr 2022 at 10:14, Ekaterina Dimitrova <[email protected]> wrote: > Hi everyone, > In Cassandra-17571 I am going to handle overflowing config parameters on > startup - we protect ourselves on converting from bigger to smaller units > by setting MAX_VALUE but this is not obvious on startup and might lead to > misunderstanding and problems. So we need to fail startup and ask for > smaller value from the very beginning. > There are two options to handle the error messaging to the users and I > would like to know from the community which one is preferred: > 1) I can add utility methods to validate that the provided value will not > overflow in the smallest allowed unit for the respective parameters and do > the checking in the Database Descriptor. The plus is we can say exactly > which parameter is not OK, the downside is maintainability because someone > might add new parameters and forget to add validations. Then the user will > fall back to assigning MAX_VALUE. We already noticed how startup checks and > some setters do not always match. > Also, David is about to push a patch JMX to intercept all Cassandra > exceptions (like the configuration one) and throw Runtime Exception so we > don’t have to worry about that anymore. > 2) After a discussion with David and Caleb I prototyped in CASSANDRA-17571 > new extended classes to the new types to handle int bounded parameters and > add checks for the mentioned problems in the constructors. This is less > error prone, validation will always happen at least for the general cases > (now if there is some special case to check for something between 0 and 10 > for example this will still go in the DatabaseDescriptor) but the issue is > that in the error message we will have only the overflowing value without > the name of the exact parameter. While this type of overflow might not be a > super common mistake(I hope), especially when we document everything, it > might still lead to inconvenience for someone so I wanted to verify this > with the community before going into one or the other direction. > > So to be more descriptive, example of produced error in both cases: > 1)" parameter1=very-big-value-in-a-bigger-unit cannot be more than > MAX_VALUE in the-smallest-unit" > 2) "very-big-value-in-a-bigger-unit cannot be more than MAX_VALUE in > the-smallest-unit" > > I think that if there is the same value more than once then the smallest > unit will guide us which one was the problem. Also, actually you will at > once check all of those same ones instead of failing on different > parameters after every correction on startup. > > Let me know what you think. If we go for option 2 I need to migrate > parameters to the prototyped extended classes before we release as that > will mean different types for affected parameters in Config class. If we > have a consensus, I can finish the rest of the work on Monday. > > Looking forward to your feedback. > Best regards, > Ekaterina >
