You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by nakomis <gi...@git.apache.org> on 2014/12/19 11:41:46 UTC
[GitHub] incubator-brooklyn pull request: Parent display name in location m...
GitHub user nakomis opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/409
Parent display name in location metadata
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nakomis/incubator-brooklyn parent-display-name-in-location-metadata
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/409.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 #409
----
commit 370187c8d1873e911dd6951649a35c548c7a32c5
Author: Martin Harris <gi...@nakomis.com>
Date: 2014-12-12T14:06:36Z
Adds catalogItemId to UsageListener.onApplicationEvent
commit c558b92ba129db570903faf179aa47e7cf5b5cd8
Author: Martin Harris <gi...@nakomis.com>
Date: 2014-12-19T10:41:17Z
Adds parentDisplayName to location event metadata
----
---
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: Parent display name in location m...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/409
---
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: Parent display name in location m...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/409#issuecomment-67829354
Thanks @nakomis - merging.
---
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: Parent display name in location m...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/409#issuecomment-67826811
PR comment addressed
---
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: Parent display name in location m...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/409#discussion_r22144184
--- Diff: core/src/main/java/brooklyn/management/internal/UsageManager.java ---
@@ -56,11 +56,11 @@
public interface UsageListener {
public static final UsageListener NOOP = new UsageListener() {
@Override public void onApplicationEvent(String applicationId, String applicationName, String entityType,
- Map<String, String> metadata, ApplicationEvent event) {}
+ String catalogItemId, Map<String, String> metadata, ApplicationEvent event) {}
@Override public void onLocationEvent(String locationId, Map<String, String> metadata, LocationEvent event) {}
};
- void onApplicationEvent(String applicationId, String applicationName, String entityType,
+ void onApplicationEvent(String applicationId, String applicationName, String entityType, String catalogItemId,
--- End diff --
For a future PR (not here)... We should do something about these method signatures. They are not good for future-proofing our API. If anyone implements the method, then we'll break their code if we try to add additional parameters in the future. We should group the parameters into a single param (e.g. an interface with getters for each of those, saying in the interface's javadoc that users are strongly discouraged from implementing it and that additional properties may be added in future releases).
Maybe the right signature is `onApplicationEvent(String applicationId, ApplicationMetadata metadata, ApplicationEvent event)`. The reason being: applicationId is the uid of the app; the metadata tells us details of the app; the event describes the state-change that is happening for the app.
The same argument goes for `onLocationEvent`.
---
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: Parent display name in location m...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/409#discussion_r22162381
--- Diff: software/base/src/test/java/brooklyn/management/usage/ApplicationUsageTrackingTest.java ---
@@ -97,11 +98,12 @@ public void testAddAndRemoveUsageListener() throws Exception {
String appId = (String) events.get(0).get(1);
String appName = (String) events.get(0).get(2);
String entityType = (String) events.get(0).get(3);
- Map<?,?> metadata = (Map<?, ?>) events.get(0).get(4);
- ApplicationEvent appEvent = (ApplicationEvent) events.get(0).get(5);
+ Map<?,?> metadata = (Map<?, ?>) events.get(0).get(5);
+ ApplicationEvent appEvent = (ApplicationEvent) events.get(0).get(6);
assertEquals(appId, app.getId(), "events="+events);
assertNotNull(appName, "events="+events);
+ assertEquals(app.getCatalogItemId(), "testCatalogItem");
--- End diff --
This doesn't look right for the test. Instead of asserting `app.getCatalogItemId()` has the right value, do you not want to assert about the value of `String catalogItemId = (String) events.get(0).get(5); assertEquals(catalogItemId, app.getCatalogItemId())`?
---
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: Parent display name in location m...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/409#issuecomment-67751305
Do you want to close https://github.com/apache/incubator-brooklyn/pull/388 - this PR includes that commit?
Or do you want to address the `sysout` there, and then we can merge the two PRs separately?
Next time, can you please include a reference to the other PR in the description of the subsequent PR that depends on 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Parent display name in location m...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/409#issuecomment-67821741
Adds the parentDisplayName field to the location metadata. This is to allow (where available) the logging of the 'Display name' of the provisioning location, i.e. the value set in `brooklyn.location.named.foo.displayName` in addition to the name (usually VM name) stored in displayName
---
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: Parent display name in location m...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/409#discussion_r22144165
--- Diff: software/base/src/test/java/brooklyn/management/usage/ApplicationUsageTrackingTest.java ---
@@ -97,11 +98,14 @@ public void testAddAndRemoveUsageListener() throws Exception {
String appId = (String) events.get(0).get(1);
String appName = (String) events.get(0).get(2);
String entityType = (String) events.get(0).get(3);
- Map<?,?> metadata = (Map<?, ?>) events.get(0).get(4);
- ApplicationEvent appEvent = (ApplicationEvent) events.get(0).get(5);
+ String catalogItemId = (String) events.get(0).get(4);
+ System.out.println(catalogItemId);
--- End diff --
Delete the `System.out`
---
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: Parent display name in location m...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/409#issuecomment-67751114
Looks good.
Could you add a reason here for why `parentDisplayName` is being added. I wonder if that will actually cover your use case (e.g. if using a `SubnetTier` then we'll get an extra layer of location object that represents the subnet, rather than the parent being the original `JcloudsLocation` or whatever.
Ping me when the stdout is removed (on skype/e-mail), and I'll merge this.
---
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.
---