spotify / cassandra-reaper

Software to run automated repairs of cassandra
235 stars 60 forks source link

use validation annotations instead of assertions, minor refactoring #67

Closed mattnworb closed 9 years ago

mattnworb commented 9 years ago

While checking out cassandra-reaper I noticed two places where the assert keyword is used to validate the configuration file. These checks can actually be moved into the configuration class itself to have dropwizard validate the property at startup - for instance if the config file has a repairIntensity > 1.0:

cassandra-reaper.yaml has an error:
  * repairIntensity must be less than or equal to 1 (was 10.9)

Jackson can also map enum fields, so there is no need to convert from a String to the RepairParallelism enum. For example if I have repairParallelism: foo in the YAML:

cassandra-reaper.yaml has an error:
  * Failed to parse configuration at: repairParallelism; foo was not one of [SEQUENTIAL, PARALLEL, DATACENTER_AWARE]
 at [Source: N/A; line: -1, column: -1] (through reference chain: com.spotify.reaper.ReaperApplicationConfiguration["repairParallelism"])

Since assertions generally aren't enabled at runtime, this means these config checks can always be applied and not just when the assertions are explicitly enabled.

(For the same reason, it might be better to use something like Preconditions.checkNotNull(...) from guava for other places where assert null != foo is used)

While making this change I noticed that there isn't really a need for the AppContext field to be static in the ReaperApplication class, and a place where a JodaTime-builtin formatter can be used.

varjoranta commented 9 years ago

Good feedback, and I learned a few new things here.

Bj0rnen commented 9 years ago

:+1: