Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/48#discussion_r15058443
--- Diff: core/src/main/java/brooklyn/entity/effector/AddSensor.java ---
@@ -38,12 +38,11 @@
@Beta
public class AddSensor<RT,T extends Sensor<RT>> implements
EntityInitializer {
protected final T sensor;
-
public static final ConfigKey<String> SENSOR_NAME =
ConfigKeys.newStringConfigKey("name");
public static final ConfigKey<Duration> SENSOR_PERIOD =
ConfigKeys.newConfigKey(Duration.class, "period", "Period, including units e.g.
1m or 5s or 200ms");
// TODO
-// public static final ConfigKey<String> SENSOR_TYPE =
ConfigKeys.newStringConfigKey("targetType");
--- End diff --
should have had a comment: the idea was to uncomment it when we were
supporting it. previously the only subclass, `SshCommandSensor` only needed to
support strings so this was unnecessary, and this commented out line
(successfully I'd say!) indicated how to proceed when other types were wanted.
i wonder could we:
* make `targetType` optional, e.g. defaulting to either `String` or `Object`
* support convenience names `string`, `object`, `int`, `double`, to make it
easier for folks to specify the type
* rework `SshCommandSensor` to make use of this field, sharing the type
coercion routines with the new classes introduced here. (alternatively, move
the `SENSOR_TYPE` field to `HttpRequestSensor`; thinking being, if this is on
the superclass, all subclasses should support it.)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
