Github user aledsage commented on a diff in the pull request:
    --- Diff: 
core/src/main/java/org/apache/brooklyn/core/sensor/ssh/ ---
    @@ -136,7 +128,7 @@ public String get() {
    -                .onFailureOrException(Functions.constant((T) null))
    +                .onFailureOrException(Functions.constant((T) "failed"))
    --- End diff --
    Why set this to "failed" rather than null?
    The risk of "failed" is that it will trigger subsequent enrichers to try to 
do something with this value (e.g. if someone wrote an SshCommandSensor to get 
part of the url, then we might subsequently use "failed" in that url - the 
downstream enricher can't tell the difference between this being a valid value 
and being the value on failed.
    If feels like the default of null is better.
    Longer term, we could make this configurable so that the default value on 
failed can be explicitly set.


Reply via email to