apache
Log warning during upgrade for existence of removed properties.
@ctubbsii said The other thing I was thinking about is that I thought we already had some start-up config validation code that would have spit out warnings about unrecognized properties already.
Could you be think of the code that tries to convert from old to new? (may have been master / manager specific)
In general, I would lean towards failing. Trying to interpret user intent usually seems to come back to being a bad thing. While it may be "unfriendly" to abort the upgrade, it does force someone to address the situation up front. Finding out that something that you thought was X and is now Z and you are unsure when Z took effect is decidedly unfriendly.
If it is not a straight conversion where we renamed a.b.foo to a directly equivalent c.d.bar, then we should not be trying to interpret user intentions. Even with a auto rename we have issues where we would ignore the original name after the conversion if the use tried to set / use the original name.
Overall, it seems like we should provide a check config / migrate config utility that can be run offline before an upgrade that does these checks - then failing if it is still incorrect would be less painful. This might be a change in the way that we have handled upgrading in the past, but may benefit users.
Could you be think of the code that tries to convert from old to new? (may have been master / manager specific)
Very possibly. I would have to do some digging. If it still exists, it would be in the server startup code path... possibly on configuration object construction, so maybe the ServerConfigurationFactory, ServerConfiguration, AccumuloConfiguration, Property, PropertyType, ServerContext, ClientContext, or some other related code (I realize this doesn't exactly narrow it down :smiley_cat: ).
In general, I would lean towards failing. Trying to interpret user intent usually seems to come back to being a bad thing.
I definitely agree with that. In this case, though, it's probably safe to just delete it. The property was a "best effort" optimization... it did not critically change anything. And, at this point, the user doesn't have a choice... so there's not really any point in failing... it has to be removed... either by us or by them.
Even if there is not consequence of just removing it - forcing them to make that change themselves has the benefit of forcing them to address something that was not having any effect. If they are okay with that, the can delete it and move on. However, if the have a "hey, wait a minute, I thought that was for...." they may figure out an alternate that meets their requirements,
@ctubbsii - This has been modified to call ConfigCheckUtil.validate like other parts of the codebase. This will log a warning for any unknown properties and throw an error for malformed or incorrect properties (wrong type, etc.). Can you re-review when you get a chance? I'd like to get this merged up.
I talked with @ctubbsii regarding this and he had two suggestions:
- This check likely makes sense for all upgrades, not just between specific versions
- Move the check to later in the upgrade process to ensure that all other upgrades are done. There could be an upgrade to the property structure or something that precludes this check from succeeding.
Oh, also, this might be useful to put in 3.1, or even 2.1 as an extra upgrade check.