You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by ahgittin <gi...@git.apache.org> on 2015/08/18 23:59:58 UTC
[GitHub] incubator-brooklyn pull request: completes the org.apache-fication...
GitHub user ahgittin opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/844
completes the org.apache-fication - PREVIEW
and tries to give nicer package structure esp in core in the process
worth opening in the IDE and looking around, to see if you like where things are. it's hard to make it perfect (and not worth a huge amount of time), so i made some pragmatic compromises in places, though i'm open to it being improved.
the main compromise was for packages we expect to be subpackaged a lot, i moved the project name afterwards (except for api) or sometimes dropped it if there's a clear primary home and the project name would be cumbersome, so we have
api/ - o.a.b.api.entity
core/ - o.a.b.entity.core
software/base/ - o.a.b.entity.java
anything else makes it really awkward to avoid split packages imho. btw i think this goes pretty far to fixing the split packages.
personally i like the feel of it when moving around the IDE. how liberating to be able to do this!
HOWEVER tests are still FAILING - wanted to get this out for preview. the test fixes shouldn't be hard, i'll finish them tomorrow.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ahgittin/incubator-brooklyn 162-refactor
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/844.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #844
----
commit bc509036738704419814ce972b52958fb5ace797
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2015-08-18T19:20:05Z
BROOKLYN-162 - refactoring core, applying org.apache package prefix and grouping more sensibly; also restructuring api package similarly
commit d326759c5508c6767b2780fcb29156150ec0a7e8
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2015-08-18T20:14:34Z
BROOKLYN-162 - apply org.apache package prefix to policy project (a few outstanding items)
commit 36db8c7671eb192e988219c5c1d66e380b0e6df5
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2015-08-18T21:12:56Z
BROOKLYN-162 - apply org.apache package prefix to software-base, tidying package names, and moving a few sensory things to core
commit 598c8ec06febe56260a293a6caca69175c271227
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2015-08-18T21:38:46Z
BROOKLYN-162 - jclouds last few package prefixes needed, and tidy in core and elsewhere related (or observed in the process)
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: completes the org.apache-fication
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/844#issuecomment-132479942
let's continue discussion of the osgi test bundles at https://github.com/apache/incubator-brooklyn/pull/810
`*.internal.*` classes in my view are internal to brooklyn; where possible this should be internal to the module (project) but if they need to cut across internal modules i'm okay with that. we can use `x-friends` to restrict this. i'm not sure if this is what is meant by "osgi semantics". no strong feelings though.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: completes the org.apache-fication
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/844#issuecomment-132478011
@hzbarcea it has built again with no errors for me locally. the failure above is definitely from an older version. are you sure you had the latest version when you saw failures?
all the runs since then have been aborted so we seem to have the same problem with infra. any news on that?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: completes the org.apache-fication
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/844
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: completes the org.apache-fication
Posted by CMoH <gi...@git.apache.org>.
Github user CMoH commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/844#issuecomment-132493175
Starting from the error above I've found a number of text references to brooklyn.entity.basic, some in comments, others in blueprint unit tests.
Running `grep -R --exclude-dir=target --exclude-dir=.git 'brooklyn.entity.basic' .` found a lot of leftover references. I'll try fixing most of them.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: completes the org.apache-fication...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/844#issuecomment-132409570
Great stuff @aledsage . I've added comments in the google doc. This builds nicely so good to merge in my view (would like one sanity check review).
Then we should:
* let you go through any further restructure ASAP (based on doc)
* restore the OSGi items (putting back into `core/src/test/dependencies` and checking in binaries gets my vote, as they hardly ever change and are just noise to build, just like the WAR files)
* do an `egrep -r '[^e].brooklyn\.([a-z]+\.)*[A-Z]'` on src folders and docs to fix class names from text which weren't picked up
* apply XSLT transformers to fix classes in persistence (and test with nested refs)
* also apply the class-rename strategy to the java lookup, with warning, so old blueprints work
Anything else?
But fortunately once we've done the first one above, I think we can let people know snapshot is stable again!
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: completes the org.apache-fication
Posted by CMoH <gi...@git.apache.org>.
Github user CMoH commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/844#issuecomment-132487109
Builds cleanly on my machine.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: completes the org.apache-fication...
Posted by aledsage <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: completes the org.apache-fication
Posted by CMoH <gi...@git.apache.org>.
Github user CMoH commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/844#issuecomment-132487550
Running it though yields the following error:
```
2015-08-19 11:16:24,205 INFO Started Brooklyn console at http://127.0.0.1:8081/, running classpath://brooklyn.war
2015-08-19 11:16:24,215 INFO Persistence disabled
2015-08-19 11:16:24,215 INFO High availability disabled
2015-08-19 11:16:27,251 WARN Error importing catalog from classpath:/brooklyn/default.catalog.bom: org.apache.brooklyn.util.exceptions.CompoundRuntimeException: Could not resolve item server:
type: brooklyn.entity.basic.EmptySoftwareProcess
name: Server
org.apache.brooklyn.util.exceptions.CompoundRuntimeException: Could not resolve item server:
type: brooklyn.entity.basic.EmptySoftwareProcess
name: Server
at org.apache.brooklyn.util.exceptions.Exceptions.create(Exceptions.java:278) ~[org.apache.brooklyn-brooklyn-utils-common-0.8.0-SNAPSHOT.jar:0.8.0-SNAPSHOT]
2015-08-19 11:16:27,252 INFO Launched Brooklyn; will now block until shutdown command received via GUI/API (recommended) or process interrupt.
```
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: completes the org.apache-fication
Posted by hzbarcea <gi...@git.apache.org>.
Github user hzbarcea commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/844#issuecomment-132444589
@aledsage excellent summary not much to add.
* +1 on the proposal to keep the OSGi semantics of 'internal'
* once this is done, there is a bit of change needed for the felix bundle plugin to properly add Import- and Export-Package with proper version ranges in the manifest. I can take care of that.
* on the OSGi test artifacts I am not sure it's a good idea to move them back to core/src/test/dependencies, and not a good idea to keep them as binaries. I am -1 on that for now. With the karaf infrastructure will come a paxexam based itest environment where they may prove useful. I would suggest we make a decision once that is in place (should be days after the refactoring). There are other solutions, btw, such as a separate git repo for 'test' or 'sample' bundles.
* the docs will require a full review indeed, I am planning on going very carefully through them once this is done.
I tested #844 locally and there are test failure. Based on the work done, I also closed #841 per @ahgittin recommendation.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: completes the org.apache-fication
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/844#issuecomment-132522638
@CMoH works great for me now. can you confirm it starts cleanly, then we can merge this, then apply your fixes to other refs to stale classes. (it's just one file which has changed so conflicts should be manageable.)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: completes the org.apache-fication
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/844#issuecomment-132516552
Ah - good find @CMoH -- I'll fix the `default.catalog.bom`. I want to get a stable PR merged ASAP, then we can fix the remaining refs, for which our various grep commands should help a lot.
@aledsage you said you had some tools which could identify how files and/or packages have moved? are those shared? (feels like there should be some piping of a `git` command and an `awk` which would do this nicely)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: completes the org.apache-fication
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/844#issuecomment-132520169
@CMoH i've pushed an update which i think should fix that one file. testing it now.
the last item in my list above should mean the old catalog would work ("also apply the class-rename strategy to the java lookup, with warning, so old blueprints work") -- but we need it working out of the box so this fix does need to be included here
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: completes the org.apache-fication
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/844#issuecomment-132544588
Confirmed w another healthy build. I'm going to merge. @CMoH let me know as soon as you have updates.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---