You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by alasdairhodge <gi...@git.apache.org> on 2016/05/12 08:25:25 UTC
[GitHub] brooklyn-server pull request: FOR REVIEW: easier access for entity...
GitHub user alasdairhodge opened a pull request:
https://github.com/apache/brooklyn-server/pull/137
FOR REVIEW: easier access for entity config by name (not ConfigKey)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/alasdairhodge/brooklyn-server easy-config-without-explicit-configkey
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/137.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 #137
----
commit 54d5f3fd34b87fabdcc952efd261e6be881c1dae
Author: Alasdair Hodge <gi...@alasdairhodge.co.uk>
Date: 2016-05-12T08:20:33Z
Provider easier access for entity config by name (not ConfigKey)
----
---
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] brooklyn-server pull request: FOR REVIEW: easier access for entity...
Posted by kiuby88 <gi...@git.apache.org>.
Github user kiuby88 commented on the pull request:
https://github.com/apache/brooklyn-server/pull/137#issuecomment-218761068
@alasdairhodge I have compiled this PR using my mac OS X 10.9.5. I do not know why Jenkins is not happy
---
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] brooklyn-server pull request #137: FOR REVIEW: easier access for entity conf...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/137#discussion_r74699774
--- Diff: core/src/main/java/org/apache/brooklyn/util/core/flags/TypeCoercions.java ---
@@ -138,6 +139,9 @@ private TypeCoercions() {}
@SuppressWarnings({ "unchecked" })
public static <T> T coerce(Object value, TypeToken<T> targetTypeToken) {
if (value==null) return null;
+ if (value instanceof DeferredSupplier<?>) {
+ value = ((DeferredSupplier<?>) value).get();
--- End diff --
@aledsage: Now that the coersion methods have been moved into `brooklyn-utils-common`, we can't implement this check as-is without adding a new dependency. `DeferredSupplier` is arguably also a "common utility" so could be moved, but that ovbiously has implications beyong this PR. Let's review.
---
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] brooklyn-server issue #137: FOR REVIEW: easier access for entity config by n...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/137
@alasdairhodge can you rebase this please so we can see if all the tests pass?
I'm still not sure about us calling `deferredSupplier.get()` inside coerce. I have a couple of big worries. First that it might have unanticipated side-effects for other code (e.g. code that is supposed to be non-blocking might now block). And second that it is only doing a small part of what `ValueResolver` does (e.g. handle `DeferredSupplier`, `Task`, `Future`, and deep inspection to resolve values inside maps, lists, etc).
The second point means that it might work in certain use-cases and then break in other use-cases. If we want the value to really be resolved, then perhaps we should go through the same code path as the existing `config().get(key)`, but with the type being passed in rather than being obtained from the type defined in the `ConfigKey`.
---
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] brooklyn-server pull request: FOR REVIEW: easier access for entity...
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on the pull request:
https://github.com/apache/brooklyn-server/pull/137#issuecomment-218762151
It seems the issue is unrelated to this PR. @neykov could you check the test failure?
---
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] brooklyn-server pull request #137: FOR REVIEW: easier access for entity conf...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/137#discussion_r71403945
--- Diff: core/src/main/java/org/apache/brooklyn/util/core/flags/TypeCoercions.java ---
@@ -138,6 +139,9 @@ private TypeCoercions() {}
@SuppressWarnings({ "unchecked" })
public static <T> T coerce(Object value, TypeToken<T> targetTypeToken) {
if (value==null) return null;
+ if (value instanceof DeferredSupplier<?>) {
+ value = ((DeferredSupplier<?>) value).get();
--- End diff --
I probably agree with this, but I worry about the implications. The `deferredSupplier.get()` call might be very slow. Are we always doing this in a context where we accept that slowness (e.g. never called by `getNonBlocking()` without first checking that it's not a deferredSupplier).
@alasdairhodge can you rebase this against master - we've fixed quite a few non-deterministic test failures, so hopefully the jenkins build will be better behaved this time.
---
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] brooklyn-server pull request #137: FOR REVIEW: easier access for entity conf...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/137#discussion_r74739189
--- Diff: core/src/main/java/org/apache/brooklyn/util/core/flags/TypeCoercions.java ---
@@ -138,6 +139,9 @@ private TypeCoercions() {}
@SuppressWarnings({ "unchecked" })
public static <T> T coerce(Object value, TypeToken<T> targetTypeToken) {
if (value==null) return null;
+ if (value instanceof DeferredSupplier<?>) {
+ value = ((DeferredSupplier<?>) value).get();
--- End diff --
@aledsage: Revisiting this, `DeferredSupplier` simply restates the contract of (Guava) `Supplier`. While superficially redundant, it may offer a (tenuous) distinction between "immediately resolvable" and "deferred-ly resolvable" quantities. Seems weak though, and potentially *very* confusing for users.
---
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] brooklyn-server pull request #137: FOR REVIEW: easier access for entity conf...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge closed the pull request at:
https://github.com/apache/brooklyn-server/pull/137
---
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.
---