Hi Carsten,
On Tue, Aug 13, 2013 at 4:11 PM, Carsten Ziegeler <[email protected]> wrote: > ...I'm not sure whether the current handling of log entries and status between > the result object and the log entries is a good thing.... Reading your comments, I agree it can be improved. > ...all log entries added to the result are logged to a logger - I > think this can easily result in having the statements logged out more than > once and it breaks the contract of having result a simple data object... Right, this is not really useful in the end, I agree about removing all the slf4j stuff in the Result class. Clients can always log the Results if they want. . > In addition, when a log message is added, it's level is evaluated to > automatically set the status - if a HC check is just doing log statements > for informational purpuse with level that leads to automatic status > setting, the HC can't set the status to a lower level afterwards... Correct, that's by design, but can be improved. I'll explain below. . > ...I think there shouldn't be any additional logic in the result class except > from just adding the log entries to a collection.... I disagree, having a minimum amount of semantics in the Result class helps standardize the way health checks report their results. We could also use a Map<Object, Object> as the result...I dont think that's what we want ;-) Now, seeing your comments I realize that the current Result doesn't express my ideas clearly enough - here are the two simple rules that I absolutely want to keep: Rule 1: Health checks *must* explain why they set the result to a given state. An HC cannot just say "my result is critical", it has to say why. Rule 2: Once a result has been set to a given state, you cannot set it back to a lower state: if you have found a critical problem, whatever happens next doesn't matter, the result is critical. This prevents subtle mistakes in HC implementations. I hope you agree that these are good rules to have, and I think we can modify the Result class to better express this: public class Result { public enum Status { DEBUG, INFO, OK, WARN, CRITICAL, HC_ERROR } /** Report an informational or warning message to this Result and optionally * change its Status. * @param reportStatus doesn't change the result's status if <= than current Status */ public void report(Status reportStatus, String message); // ...getStatus, isOk, merge and iterator() unchanged, others methods removed } Initial status is OK, so DEBUG and INFO are never reported as the result's Status, it starts at OK. The setStatus() method is gone, you have to go via report(...) to force an explanation. Using the word "report" might be better to avoid confusion with logs, so we can also rename ResultLogEntry to ReportEntry, and it now has a Status and a message. Does that work for you, or do you have a better proposal that respects the above two rules? -Bertrand
