clean up API

[mhaack]

Refactor and clean up API package t avoid duplicated code and be more straight forward

• both classes (HealthStatus and SystemHealthStatus) hold status and have similar/copied methods (getStatus)

This should be refactored in the way

• make SystemHealthStatus inherit from HealthStatus
• or use an common interface and abstract base class

WDYT?

[joerghoh]

I don't think that HealthStatus and SystemHealthStatus have that much in common besides the name (getStatusText and GetStatus look very similar, indeed).

They also serve different purposes, the SystemHealthStatus being an overall status containing a list of smaller healthstatus results.

So -1 for creating inheritance between these 2 classes.

[alexsaar]

I'm not sure yet. but created a branch to try something out.

+0 for now. let's see where this goes

[mhaack]

Ok, I agree with Jörg not to have inheritance between these 2 classes, but still like the idea of a common base class to merge down the status field, getStatusText & getStatus methods (have different names but do the same) to avoid code duplication.

I also like to vote for getting rid of the text representation of the status in the java code completely and only translate the status code to text in the ui.

[alexsaar]

I pushed 2 new branches.

• joerghoh/cq5-healthcheck/tree/15-APICleanup contains a first step that introduces a common base class for shared methods etc
• joerghoh/cq5-healthcheck/tree/15-1-APICleanup goes one step further and merges the status impls completely

I personally prefer the second version as it makes the API more slick and easier to understand although a status object is a bit more complex now.

let me know what you think

[mhaack]

+1 for second approach, that is exactly what I was thinking of

[alexsaar]

@joerghoh we have 2 +1 and your -1 now. Would you like to reconsider?

[joerghoh]

Well, I reviewed both approaches. Branch 15-APICleanup looks reasonable, as it extracts the Status and StatusCode aspects into separate classes and makes it available via both SystemStatus and HealthStatus.

In branch 15-1-APICleanup the distinction between SystemStatus and HealthStatus is no longer visible, as both aspects are merged into a single class and having 4 (!!) different constructors.

So +1 for 15-APICleanup

[alexsaar]

why is the number of constructors a problem? we could also provide setter methods and reduce the number of constructors. but in the end they are just utilities and everyone can use the one that he needs.

the distinction between SystemStatus and HealthStatus is imho opinion not necessary. a status is a status is a status :)

the difference in the current model is that SystemStatus can have nested HealthStatus objects. the new status class provides the same functionality by building a tree of status objects and the SystemStatus is just the root of the tree. and we can group status objects by nesting them further which allows for more flexibility in future.

[joerghoh]

Ah, you're introducing a new feature "nested status"? Can we focus on this task on "refactoring" and postpone the new feature discussion to a new task?

[alexsaar]

It would help to implement suggestion from @mhaack in #13 which would also allow disabling multiple checks at once by disabling just a single service.

Other ideas are to group status objects, eg

Overall Status contains

• agents
  • publish
  • reverse

and so on. Or we could think of combined status calculations that make use of different MBean properties and only if all of the parameters/conditions match the status changes (eg number of stale workflows > 10 and publish queue > 40). For such we would have a combined status that would also allow to drill down into the details.

all this is of course still subject to discussion and future implementation. but the proposed nested status would also modeling this without major API changes (may need some additional functions though).

[mhaack]

I agree with @joerghoh, lets move the feature discussion to another issue.

Back to the API clean up, after reviewing both approaches again, for me it is a weighing between a minimal API (with some functionality not visible at first sight) in 15-1-APICleanup and a more expressive API with the with distinction between SystemStatus and HealthStatus in 15-APICleanup.

I'm personally fine with both, also have no problem with the overloading of the constructors. But I think for others it might be easier to understand to have the distinction between SystemStatus and HealthStatus.

So if I'm allowed ;) I change my opinion and move my vote to option 15-APICleanup.

[alexsaar]

as discussed w/ @mhaack and @joerghoh we now decided to go with the second approach to be more flexible in future