Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/844#issuecomment-132400477
Thanks @ahgittin - much clearer!
I've written up my comments at
https://docs.google.com/document/d/1un7SwvoE43lWaIlD5CRdIdza4XJkHji-OhTKXpkr0NA,
because there are a lot of them!
We can do this package name incrementally - but agree we want to get most
of the changes done quickly in a small number of steps to limit the disruption
for package renames causing merge conflicts, and problems for downstream
projects trying to keep up.
Comments copy-pasted below for posterity, but recommend you use the link
above for a better formatted doc:
General questions
* What is our public API?
How do we communicate to users what is internal impl, versus public API?
Do we just list packages that are âpublicâ?
We definitely want a set of packages that we are free to refactor,
without deprecation warnings (or worries about persisted state!).
* What does âinternalâ mean in the package name?
Is this to indicate that it is internal to the module (i.e. OSGi meaning)?
Or just not part of the public API?
It would be nice if we can use the OSGi meaning of âinternalâ.
*api*
EntityLocal:
move to org.apache.brooklyn.api.entity, and deprecate it!
BrooklynClassLoadingContext:
could move to core? note referenced by rest of API?
org.apache.brooklyn.api.sensor.Enricher*:
I'd put this in org.apache.brooklyn.api.enricher
We talk about policies and enrichers, so deserves a top-level I think.
org.apache.brooklyn.api.sensor.Feed:
I'd (maybe) put this in org.apache.brooklyn.api.feed
no strong feelings.
org.apache.brooklyn.api.mgmt.*Task*
could move to
org.apache.brooklyn.api.task
Iâm inclined to move all the basic task stuff into a separate
âtaskâ module, in time.
To deprecate, includes:
org.apache.brooklyn.api.mgmt.rebind.RebindSupport
org.apache.brooklyn.api.mgmt.rebind.Rebindable
org.apache.brooklyn.core.mgmt.internal.LocalAccessManager
org.apache.brooklyn.api.mgmt.AccessController
*core*
org.apache.brooklyn.camp.brooklyn.api
Looks out of place in core.
org.apache.brooklyn.camp.brooklyn.api.HasBrooklynManagementContext
Feels like it should sit near ManagementContextInjectable
Feels like nothing to do with camp.
*.internal:
Is this to indicate that it is internal to the module? Or just not part
of the public API?
Would be good if we can use the OSGi meaning of âinternalâ.
org.apache.brooklyn.core.internal.BrooklynFeatureEnablement
Does feel internal to core
org.apache.brooklyn.core.mgmt.internal.UsageListener & UsageManager:
Suspect this is part of public API.
Used by customers / power users to track their customersâ usage.
Could move into org.apache.brooklyn.core.mgmt.usage (need to double check)
.rebind versus .persist:
What is the difference?
Should .persist contain everything for persisting? And .rebind just what is
used for rebinding?
Longer term⦠could separate RebindManager from PersistenceManager
(but not now)
org.apache.brooklyn.effector.core:
Iâd rename it org.apache.brooklyn.core.effector
Recommend moving AddEffector, AddSensor and AddChildrenEffector.
e.g. beside EntityInitializers
Maybe in org.apache.brooklyn.core.entity.initializers
org.apache.brooklyn.entity.*
Uncomfortable with this.
There are quite a few sub-packages; so change of a name collision with
something in a brooklyn-software-* module seems risky.
Also, makes it hard to tell what is core from the package name.
Iâd go for org.apache.brooklyn.core.entity.* for anything about writing
entities or helpers.
This would include:
org.apache.brooklyn.core.entity
org.apache.brooklyn.core.entity.drivers.*
org.apache.brooklyn.core.entity.factory
org.apache.brooklyn.core.entity.internal
org.apache.brooklyn.core.entity.lifecycle
org.apache.brooklyn.core.entity.trait
org.apache.brooklyn.core.entity.trait
Iâm more happy with things that are actually entities:
org.apache.brooklyn.entity.group
org.apache.brooklyn.entity.stock
org.apache.brooklyn.entity.annotation:
Uncomfortable with this.
Contains only a couple of annotations; other annotations are elsewhere
(e.g. org.apache.brooklyn.api.catalog.CatalogConfig)
org.apache.brooklyn.location.*:
Uncomfortable with this, as with org.apache.brooklyn.entity.*
Iâd name them:
org.apache.brooklyn.core.location
org.apache.brooklyn.core.location.access
org.apache.brooklyn.core.location.cloud
org.apache.brooklyn.core.location.dynamic
org.apache.brooklyn.core.location.geo
org.apache.brooklyn.core.location.dynamic
org.apache.brooklyn.core.location.dynamic
org.apache.brooklyn.core.location.dynamic
Iâm fine with:
org.apache.brooklyn.location.byon
org.apache.brooklyn.location.localhost
org.apache.brooklyn.location.ssh
org.apache.brooklyn.location.winrm
org.apache.brooklyn.location.paas:
Letâs deprecate or even delete PaasLocation - and resurrect it
differently when we can deploy to Cloud Foundry.
org.apache.brooklyn.policy.core:
rename to org.apache.brooklyn.core.policy
org.apache.brooklyn.sensor.core:
rename to org.apache.brooklyn.core.sensor
org.apache.brooklyn.sensor.enricher:
could rename to org.apache.brooklyn.enricher.stock
(see comment under api about not having this inside âsensorâ)
and separate out anything that is a utility for writing new enrichers
etc into:
org.apache.brooklyn.core.enricher
(e.g. AbstractEnricher, etc)
org.apache.brooklyn.sensor.feed:
Iâd put the core feed stuff in org.apache.brooklyn.core.feed
For particular feed impls, Iâm fine with us having:
org.apache.brooklyn.feed.function
org.apache.brooklyn.feed.http
etc
org.apache.brooklyn.util.core:
Iâd go for org.apache.brooklyn.core.util
And we should more carefully review whatâs in here, to see what to
move/delete.
org.apache.brooklyn.util.core.task:
Perhaps put this in org.apache.brooklyn.util.task
Reasoning is that we may well move the task stuff into its own module.
(core is way too big!)
However, o.a.b.core.util.task.ssh etc is fine, as these are not for
generic tasks.
*software-base*
org.apache.brooklyn.entity.system_service:
not sure about this (itâs too late at night now!)
org.apache.brooklyn.sensor.ssh:
2 of 3 of its classes are not related to sensors.
Could be an âinitializersâ package?
SshEffectorTasks: should that be in core, closer to
org.apache.brooklyn.util.core.task.ssh?
org.apache.brooklyn.sensor.feed.jmx:
Could be renamed to org.apache.brooklyn.feed.jmx?
org.apache.brooklyn.sensor.winrm:
Uncomfortable with this namespace.
*brooklyn-utils-common*
org.apache.brooklyn.util:
this package name is also in brooklyn-utils-groovy
*brooklyn-utils-test-support*
org.apache.brooklyn.test:
this package name is also in brooklyn-utils-common
Rename this one, e.g. to org.apache.brooklyn.test.support
---
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.
---
