Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/853#issuecomment-132787404
@aledsage great stuff here
two changes to this I'd like to see, if you're okay with them:
*
`software/base/src/main/java/org/apache/brooklyn/sensor/ssh/SshEffectorTasks.java`
now in `core`, `org/apache/brooklyn/util/core/task/ssh/`: both seem wrong;
unlike the rest of `util/core` which has only minimal dependencies on the rest
of core, this is designed to create effectors; so maybe `core`,
`org/apache/brooklyn/effector/core` is best?
* in `utils/groovy/src/main/java/org/apache/brooklyn/util/groovy/internal/`
can we drop the `internal` while we're doing this?
i'd also like to try to draw a line under refactoring as much as possible
soon, as it is disruptive for people. (should be far less so now, but even
so!) i like the way `o.a.b.entity.*` is entities and same for `location` and
`policy`, whereas supporting classes are `o.a.b.core.{entity,location,policy}`.
with this in mind i'm no thinking:
* it makes sense with effectors, since everything is support, for them to
be under `o.a.b.core.effector`
* with sensors likewise, supporting things going in `o.a.b.core.sensor`
makes sense; the feeds and enrichers i'm on the fence.
`o.a.b.core.sensor.feed.*` is long-winded but okay. or any of `o.a.b.feed` or
`o.a.b.feed` or `o.a.b.sensor.feed` or `o.a.b.core.feed` seem fine (and same
for `enricher`), no strong feelings, maybe a slight preference for the last
ones.
any other areas which you think need major attention?
(btw i see that you've changed references in yaml etc wrt this PR, all
looks fine. i'm happy for this to be merged before the above suggestions or
after, as you see fit. big improvement.)
---
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.
---
