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 2014/10/09 06:43:00 UTC
[GitHub] incubator-brooklyn pull request: Some further tidies in wake of la...
GitHub user ahgittin opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/233
Some further tidies in wake of latest location leak
A few things following on from #229 , mainly in `LocationRegistry` API. @neykov can you cast your eye over this please?
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ahgittin/incubator-brooklyn location-leak-tidy
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/233.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 #233
----
commit a0f5a87c8b687714e70e9fea2bc4fad71f42151c
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2014-10-09T01:29:38Z
tidy up of logging for persistence and location management
commit d1b0841d259c035d10a04d88b24882c1b28ee42f
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2014-10-09T04:23:44Z
cleans up some of the mess in LocationRegistry around testing-vs-resolving and managed-vs-unmanaged, deprecating many methods (and removing their calls)
----
---
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: Some further tidies in wake of la...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/233#discussion_r18968148
--- Diff: core/src/main/java/brooklyn/location/basic/BasicLocationRegistry.java ---
@@ -190,52 +194,58 @@ void disablePersistence() {
// defining the format and file etc)
}
- BasicLocationDefinition localhost(String id) {
+ static BasicLocationDefinition localhost(String id) {
--- End diff --
sure `protected` is fine
---
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: Some further tidies in wake of la...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/233#discussion_r18968001
--- Diff: core/src/main/java/brooklyn/management/internal/LocalLocationManager.java ---
@@ -205,11 +206,15 @@ protected Location manageRecursive(Location loc, final ManagementTransitionMode
throw new IllegalStateException("Access controller forbids management of "+loc+": "+access.getMsg());
}
- String msg = "Managing location " + loc + ". initialMode=" + initialMode + ", context: " + Strings.toString(Tasks.current());
- if (LOCATION_CNT.getAndIncrement() % 100 == 0) {
- log.debug(msg, new Exception("Stack Trace"));
- } else {
- log.debug(msg);
+ if (log.isDebugEnabled()) {
+ String msg = "Managing location " + loc + " ("+initialMode+"), from " + Tasks.current()+" / "+Entitlements.getEntitlementContext();
+ long count = LOCATION_CNT.incrementAndGet();
--- End diff --
+1
---
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: Some further tidies in wake of la...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/233#issuecomment-59391891
@grkvlt hope it helps not accidentally introducing memory leaks around dynamic locations. it's still too easily done, but this helps a bit.
---
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: Some further tidies in wake of la...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/233#discussion_r18967966
--- Diff: core/src/main/java/brooklyn/location/basic/BasicLocationRegistry.java ---
@@ -190,52 +194,58 @@ void disablePersistence() {
// defining the format and file etc)
}
- BasicLocationDefinition localhost(String id) {
+ static BasicLocationDefinition localhost(String id) {
return new BasicLocationDefinition(id, "localhost", "localhost", null);
}
/** to catch circular references */
protected ThreadLocal<Set<String>> specsSeen = new ThreadLocal<Set<String>>();
- @Override
+ @Override @Deprecated
public boolean canMaybeResolve(String spec) {
return getSpecResolver(spec) != null;
}
@Override
public final Location resolve(String spec) {
- return resolve(spec, new MutableMap());
+ return resolve(spec, true, null).get();
}
- @Override
+ @Override @Deprecated
public final Location resolveIfPossible(String spec) {
if (!canMaybeResolve(spec)) return null;
- try {
- return resolve(spec);
- } catch (Exception e) {
- if (log.isTraceEnabled())
- log.trace("Unable to resolve "+spec+": "+e, e);
- // can't resolve
- return null;
- }
+ return resolve(spec, null, null).orNull();
}
-
- @Override
- public Location resolve(String spec, Map locationFlags) {
+
+ @Deprecated /** since 0.7.0 not used */
+ public Maybe<Location> resolve(String spec, boolean manage) {
+ return resolve(spec, manage, null);
+ }
+
+ public Maybe<Location> resolve(String spec, Boolean manage, Map locationFlags) {
try {
+ if (locationFlags==null) locationFlags = MutableMap.of();
+ if (manage!=null) {
+ locationFlags.put(LocalLocationManager.CREATE_UNMANAGED, !manage);
--- End diff --
+1
---
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: Some further tidies in wake of la...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/233#discussion_r18630907
--- Diff: api/src/main/java/brooklyn/location/LocationRegistry.java ---
@@ -49,35 +51,54 @@
/** removes the defined location from the registry (applications running there are unaffected) */
public void removeDefinedLocation(String id);
- /** returns fully populated (config etc) location from the given definition,
- * currently by creating it but TODO creation can be a leak so all current 'resolve' methods should be carefully used! */
+ /** Returns a fully populated (config etc) location from the given definition, with optional add'l flags.
+ * the location will be managed by default, unless the manage parameter is false or the CREATE_UNMANAGED flag is set.
+ * @since 0.7.0, but beta and likely to change as the semantics of this class are tuned */
+ @Beta
+ public Maybe<Location> resolve(LocationDefinition ld, Boolean manage, Map locationFlags);
--- End diff --
What's the point of using Boolean (vs boolean)?
Better use boolean and set CREATE_UNMANAGED only on true.
---
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: Some further tidies in wake of la...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/233#discussion_r18967476
--- Diff: api/src/main/java/brooklyn/location/LocationRegistry.java ---
@@ -49,35 +51,54 @@
/** removes the defined location from the registry (applications running there are unaffected) */
public void removeDefinedLocation(String id);
- /** returns fully populated (config etc) location from the given definition,
- * currently by creating it but TODO creation can be a leak so all current 'resolve' methods should be carefully used! */
+ /** Returns a fully populated (config etc) location from the given definition, with optional add'l flags.
+ * the location will be managed by default, unless the manage parameter is false or the CREATE_UNMANAGED flag is set.
+ * @since 0.7.0, but beta and likely to change as the semantics of this class are tuned */
+ @Beta
+ public Maybe<Location> resolve(LocationDefinition ld, Boolean manage, Map locationFlags);
--- End diff --
`null` for `manage` meant to rely on the flags, and manage if no flags; that's the default used in most places. but fair enough to say `true` means leave unset (so rely on flags). will change to `boolean`.
---
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: Some further tidies in wake of la...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/233#issuecomment-59392138
addressed review comments, and rebased. 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: Some further tidies in wake of la...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/233#discussion_r18744011
--- Diff: core/src/main/java/brooklyn/location/basic/BasicLocationRegistry.java ---
@@ -190,52 +194,58 @@ void disablePersistence() {
// defining the format and file etc)
}
- BasicLocationDefinition localhost(String id) {
+ static BasicLocationDefinition localhost(String id) {
return new BasicLocationDefinition(id, "localhost", "localhost", null);
}
/** to catch circular references */
protected ThreadLocal<Set<String>> specsSeen = new ThreadLocal<Set<String>>();
- @Override
+ @Override @Deprecated
public boolean canMaybeResolve(String spec) {
return getSpecResolver(spec) != null;
}
@Override
public final Location resolve(String spec) {
- return resolve(spec, new MutableMap());
+ return resolve(spec, true, null).get();
}
- @Override
+ @Override @Deprecated
public final Location resolveIfPossible(String spec) {
if (!canMaybeResolve(spec)) return null;
- try {
- return resolve(spec);
- } catch (Exception e) {
- if (log.isTraceEnabled())
- log.trace("Unable to resolve "+spec+": "+e, e);
- // can't resolve
- return null;
- }
+ return resolve(spec, null, null).orNull();
}
-
- @Override
- public Location resolve(String spec, Map locationFlags) {
+
+ @Deprecated /** since 0.7.0 not used */
+ public Maybe<Location> resolve(String spec, boolean manage) {
+ return resolve(spec, manage, null);
+ }
+
+ public Maybe<Location> resolve(String spec, Boolean manage, Map locationFlags) {
--- End diff --
What is the rationale behind the `final`-ness or not of the resolve methods?
---
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: Some further tidies in wake of la...
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/233#discussion_r18645376
--- Diff: core/src/main/java/brooklyn/location/basic/BasicLocationRegistry.java ---
@@ -190,52 +194,58 @@ void disablePersistence() {
// defining the format and file etc)
}
- BasicLocationDefinition localhost(String id) {
+ static BasicLocationDefinition localhost(String id) {
return new BasicLocationDefinition(id, "localhost", "localhost", null);
}
/** to catch circular references */
protected ThreadLocal<Set<String>> specsSeen = new ThreadLocal<Set<String>>();
- @Override
+ @Override @Deprecated
public boolean canMaybeResolve(String spec) {
return getSpecResolver(spec) != null;
}
@Override
public final Location resolve(String spec) {
- return resolve(spec, new MutableMap());
+ return resolve(spec, true, null).get();
}
- @Override
+ @Override @Deprecated
public final Location resolveIfPossible(String spec) {
if (!canMaybeResolve(spec)) return null;
- try {
- return resolve(spec);
- } catch (Exception e) {
- if (log.isTraceEnabled())
- log.trace("Unable to resolve "+spec+": "+e, e);
- // can't resolve
- return null;
- }
+ return resolve(spec, null, null).orNull();
}
-
- @Override
- public Location resolve(String spec, Map locationFlags) {
+
+ @Deprecated /** since 0.7.0 not used */
+ public Maybe<Location> resolve(String spec, boolean manage) {
+ return resolve(spec, manage, null);
+ }
+
+ public Maybe<Location> resolve(String spec, Boolean manage, Map locationFlags) {
try {
+ if (locationFlags==null) locationFlags = MutableMap.of();
+ if (manage!=null) {
+ locationFlags.put(LocalLocationManager.CREATE_UNMANAGED, !manage);
--- End diff --
Agree with @neykov . Unfortunately Brooklyn is littered with places that we modify the caller's map that they passed in (particularly in the `ExecutionManager` code).
This part is definitely not performance critical, so far better to take a copy.
---
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: Some further tidies in wake of la...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/233#discussion_r18743989
--- Diff: core/src/main/java/brooklyn/location/basic/BasicLocationRegistry.java ---
@@ -190,52 +194,58 @@ void disablePersistence() {
// defining the format and file etc)
}
- BasicLocationDefinition localhost(String id) {
+ static BasicLocationDefinition localhost(String id) {
--- End diff --
Dislike package scope, is it really needed or can we not be `protected` or even just `public`?
---
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: Some further tidies in wake of la...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/233
---
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: Some further tidies in wake of la...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/233#discussion_r18630931
--- Diff: core/src/main/java/brooklyn/location/basic/BasicLocationRegistry.java ---
@@ -190,52 +194,58 @@ void disablePersistence() {
// defining the format and file etc)
}
- BasicLocationDefinition localhost(String id) {
+ static BasicLocationDefinition localhost(String id) {
return new BasicLocationDefinition(id, "localhost", "localhost", null);
}
/** to catch circular references */
protected ThreadLocal<Set<String>> specsSeen = new ThreadLocal<Set<String>>();
- @Override
+ @Override @Deprecated
public boolean canMaybeResolve(String spec) {
return getSpecResolver(spec) != null;
}
@Override
public final Location resolve(String spec) {
- return resolve(spec, new MutableMap());
+ return resolve(spec, true, null).get();
}
- @Override
+ @Override @Deprecated
public final Location resolveIfPossible(String spec) {
if (!canMaybeResolve(spec)) return null;
- try {
- return resolve(spec);
- } catch (Exception e) {
- if (log.isTraceEnabled())
- log.trace("Unable to resolve "+spec+": "+e, e);
- // can't resolve
- return null;
- }
+ return resolve(spec, null, null).orNull();
}
-
- @Override
- public Location resolve(String spec, Map locationFlags) {
+
+ @Deprecated /** since 0.7.0 not used */
+ public Maybe<Location> resolve(String spec, boolean manage) {
+ return resolve(spec, manage, null);
+ }
+
+ public Maybe<Location> resolve(String spec, Boolean manage, Map locationFlags) {
try {
+ if (locationFlags==null) locationFlags = MutableMap.of();
+ if (manage!=null) {
+ locationFlags.put(LocalLocationManager.CREATE_UNMANAGED, !manage);
--- End diff --
Should copy the map before mutating. Could be immutable, in any case the caller doesn't expect the flags to change.
---
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: Some further tidies in wake of la...
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/233#discussion_r18645673
--- Diff: core/src/main/java/brooklyn/management/internal/LocalLocationManager.java ---
@@ -205,11 +206,15 @@ protected Location manageRecursive(Location loc, final ManagementTransitionMode
throw new IllegalStateException("Access controller forbids management of "+loc+": "+access.getMsg());
}
- String msg = "Managing location " + loc + ". initialMode=" + initialMode + ", context: " + Strings.toString(Tasks.current());
- if (LOCATION_CNT.getAndIncrement() % 100 == 0) {
- log.debug(msg, new Exception("Stack Trace"));
- } else {
- log.debug(msg);
+ if (log.isDebugEnabled()) {
+ String msg = "Managing location " + loc + " ("+initialMode+"), from " + Tasks.current()+" / "+Entitlements.getEntitlementContext();
+ long count = LOCATION_CNT.incrementAndGet();
--- End diff --
I'd do the increment outside of the `if (log.isDebugEnabled)`. The call is cheap, and if the log level gets changed on-the-fly to debug then we want the message to have an accurate call count.
---
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: Some further tidies in wake of la...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/233#discussion_r18744023
--- Diff: core/src/main/java/brooklyn/location/basic/BasicLocationRegistry.java ---
@@ -190,52 +194,58 @@ void disablePersistence() {
// defining the format and file etc)
}
- BasicLocationDefinition localhost(String id) {
+ static BasicLocationDefinition localhost(String id) {
return new BasicLocationDefinition(id, "localhost", "localhost", null);
}
/** to catch circular references */
protected ThreadLocal<Set<String>> specsSeen = new ThreadLocal<Set<String>>();
- @Override
+ @Override @Deprecated
public boolean canMaybeResolve(String spec) {
return getSpecResolver(spec) != null;
}
@Override
public final Location resolve(String spec) {
- return resolve(spec, new MutableMap());
+ return resolve(spec, true, null).get();
}
- @Override
+ @Override @Deprecated
public final Location resolveIfPossible(String spec) {
if (!canMaybeResolve(spec)) return null;
- try {
- return resolve(spec);
- } catch (Exception e) {
- if (log.isTraceEnabled())
- log.trace("Unable to resolve "+spec+": "+e, e);
- // can't resolve
- return null;
- }
+ return resolve(spec, null, null).orNull();
}
-
- @Override
- public Location resolve(String spec, Map locationFlags) {
+
+ @Deprecated /** since 0.7.0 not used */
+ public Maybe<Location> resolve(String spec, boolean manage) {
+ return resolve(spec, manage, null);
+ }
+
+ public Maybe<Location> resolve(String spec, Boolean manage, Map locationFlags) {
try {
+ if (locationFlags==null) locationFlags = MutableMap.of();
+ if (manage!=null) {
+ locationFlags.put(LocalLocationManager.CREATE_UNMANAGED, !manage);
+ }
+
Set<String> seenSoFar = specsSeen.get();
if (seenSoFar==null) {
seenSoFar = new LinkedHashSet<String>();
specsSeen.set(seenSoFar);
}
if (seenSoFar.contains(spec))
- throw new IllegalStateException("Circular reference in definition of location '"+spec+"' ("+seenSoFar+")");
+ return Maybe.absent(Suppliers.ofInstance(new IllegalStateException("Circular reference in definition of location '"+spec+"' ("+seenSoFar+")")));
--- End diff --
Could we not use the `Maybe#absent(String)` with the error message, which should have an identical effect?
---
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: Some further tidies in wake of la...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/233#issuecomment-58763022
Looks useful for Clocker which needs to create a lot of locations on the fly :frog:
---
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: Some further tidies in wake of la...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/233#issuecomment-58476911
The API is way better this way, tests are passing for me. Other than the two points above the changes look great.
---
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: Some further tidies in wake of la...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/233#discussion_r18968370
--- Diff: core/src/main/java/brooklyn/location/basic/BasicLocationRegistry.java ---
@@ -190,52 +194,58 @@ void disablePersistence() {
// defining the format and file etc)
}
- BasicLocationDefinition localhost(String id) {
+ static BasicLocationDefinition localhost(String id) {
return new BasicLocationDefinition(id, "localhost", "localhost", null);
}
/** to catch circular references */
protected ThreadLocal<Set<String>> specsSeen = new ThreadLocal<Set<String>>();
- @Override
+ @Override @Deprecated
public boolean canMaybeResolve(String spec) {
return getSpecResolver(spec) != null;
}
@Override
public final Location resolve(String spec) {
- return resolve(spec, new MutableMap());
+ return resolve(spec, true, null).get();
}
- @Override
+ @Override @Deprecated
public final Location resolveIfPossible(String spec) {
if (!canMaybeResolve(spec)) return null;
- try {
- return resolve(spec);
- } catch (Exception e) {
- if (log.isTraceEnabled())
- log.trace("Unable to resolve "+spec+": "+e, e);
- // can't resolve
- return null;
- }
+ return resolve(spec, null, null).orNull();
}
-
- @Override
- public Location resolve(String spec, Map locationFlags) {
+
+ @Deprecated /** since 0.7.0 not used */
+ public Maybe<Location> resolve(String spec, boolean manage) {
+ return resolve(spec, manage, null);
+ }
+
+ public Maybe<Location> resolve(String spec, Boolean manage, Map locationFlags) {
--- End diff --
in general, anything that shouldn't be overridden because it is a tiny wrapper around another method, makes sense to be final. it was haphazard, i think to prevent problems in tests which overrode methods. have made it more consistent.
---
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: Some further tidies in wake of la...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/233#discussion_r18968481
--- Diff: core/src/main/java/brooklyn/location/basic/BasicLocationRegistry.java ---
@@ -190,52 +194,58 @@ void disablePersistence() {
// defining the format and file etc)
}
- BasicLocationDefinition localhost(String id) {
+ static BasicLocationDefinition localhost(String id) {
return new BasicLocationDefinition(id, "localhost", "localhost", null);
}
/** to catch circular references */
protected ThreadLocal<Set<String>> specsSeen = new ThreadLocal<Set<String>>();
- @Override
+ @Override @Deprecated
public boolean canMaybeResolve(String spec) {
return getSpecResolver(spec) != null;
}
@Override
public final Location resolve(String spec) {
- return resolve(spec, new MutableMap());
+ return resolve(spec, true, null).get();
}
- @Override
+ @Override @Deprecated
public final Location resolveIfPossible(String spec) {
if (!canMaybeResolve(spec)) return null;
- try {
- return resolve(spec);
- } catch (Exception e) {
- if (log.isTraceEnabled())
- log.trace("Unable to resolve "+spec+": "+e, e);
- // can't resolve
- return null;
- }
+ return resolve(spec, null, null).orNull();
}
-
- @Override
- public Location resolve(String spec, Map locationFlags) {
+
+ @Deprecated /** since 0.7.0 not used */
+ public Maybe<Location> resolve(String spec, boolean manage) {
+ return resolve(spec, manage, null);
+ }
+
+ public Maybe<Location> resolve(String spec, Boolean manage, Map locationFlags) {
try {
+ if (locationFlags==null) locationFlags = MutableMap.of();
+ if (manage!=null) {
+ locationFlags.put(LocalLocationManager.CREATE_UNMANAGED, !manage);
+ }
+
Set<String> seenSoFar = specsSeen.get();
if (seenSoFar==null) {
seenSoFar = new LinkedHashSet<String>();
specsSeen.set(seenSoFar);
}
if (seenSoFar.contains(spec))
- throw new IllegalStateException("Circular reference in definition of location '"+spec+"' ("+seenSoFar+")");
+ return Maybe.absent(Suppliers.ofInstance(new IllegalStateException("Circular reference in definition of location '"+spec+"' ("+seenSoFar+")")));
--- End diff --
**No!** If you do `absent(String)` you get the message you want, but the trace is when the person does a `get()`. Here I want the trace (or cause) to be from this class.
---
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: Some further tidies in wake of la...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/233#discussion_r18744035
--- Diff: core/src/main/java/brooklyn/location/basic/BasicLocationRegistry.java ---
@@ -263,13 +273,18 @@ public Location resolve(String spec, Map locationFlags) {
+ "either this location is not recognised or there is a problem with location resolver configuration.";
}
- throw new NoSuchElementException(errmsg);
+ return Maybe.absent(Suppliers.ofInstance(new NoSuchElementException(errmsg)));
--- End diff --
Same as above, except use `Maybe#absent(Throwable)` with the `NoSuchElementException` instance?
---
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: Some further tidies in wake of la...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/233#discussion_r18744054
--- Diff: core/src/main/java/brooklyn/location/basic/BasicLocationRegistry.java ---
@@ -343,22 +358,27 @@ public Location resolveForPeeking(LocationDefinition ld) {
return resolve(ld, ConfigBag.newInstance().configure(LocalLocationManager.CREATE_UNMANAGED, true).getAllConfig());
}
- @Override
+ @Override @Deprecated
public Location resolve(LocationDefinition ld, Map<?,?> flags) {
return resolveLocationDefinition(ld, flags, null);
}
+ /** @deprecated since 0.7.0 not used (and optionalName was ignored anyway) */
+ @Deprecated
public Location resolveLocationDefinition(LocationDefinition ld, Map locationFlags, String optionalName) {
+ return resolve(ld, null, locationFlags).get();
+ }
+
+ public Maybe<Location> resolve(LocationDefinition ld, Boolean manage, Map locationFlags) {
ConfigBag newLocationFlags = ConfigBag.newInstance(ld.getConfig())
.putAll(locationFlags)
.putIfAbsentAndNotNull(LocationInternal.NAMED_SPEC_NAME, ld.getName())
.putIfAbsentAndNotNull(LocationInternal.ORIGINAL_SPEC, ld.getName());
- try {
- return resolve(ld.getSpec(), newLocationFlags.getAllConfig());
- } catch (Exception e) {
- throw new IllegalStateException("Cannot instantiate location '"+
- (optionalName!=null ? optionalName : ld)+"' pointing at "+ld.getSpec()+": "+e, e);
- }
+ Maybe<Location> result = resolve(ld.getSpec(), manage, newLocationFlags.getAllConfigRaw());
+ if (result.isPresent())
+ return result;
+ throw new IllegalStateException("Cannot instantiate location '"+ld+"' pointing at "+ld.getSpec()+": "+
+ Exceptions.collapseText( ((Absent<?>)result).getException() ));
--- End diff --
I wish there was a nicer way of getting the exception from a `Maybe.Absent` instance, without the cast...
---
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: Some further tidies in wake of la...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/233#discussion_r18743986
--- Diff: core/src/main/java/brooklyn/entity/rebind/persister/BrooklynMementoPersisterToObjectStore.java ---
@@ -522,13 +522,12 @@ public void delta(Delta delta, PersistenceExceptionHandler exceptionHandler) {
Stopwatch stopwatch = deltaImpl(delta, exceptionHandler);
- if (LOG.isDebugEnabled()) LOG.debug("Checkpointed delta of memento in {}; updated {} entities, {} locations, " +
- "{} policies, {} enrichers and {} catalog items; " +
- "removing {} entities, {} locations, {} policies, {} enrichers and {} catalog items",
- new Object[] {Time.makeTimeStringRounded(stopwatch),
+ if (LOG.isDebugEnabled()) LOG.debug("Checkpointed delta of memento in {}: "
+ + "updated {} entities, {} locations, {} policies, {} enrichers, {} catalog items; "
+ + "removed {} entities, {} locations, {} policies, {} enrichers {} catalog items",
--- End diff --
Insert another comma: `enrichers, {} catalog items` 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: Some further tidies in wake of la...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/233#discussion_r18967956
--- Diff: api/src/main/java/brooklyn/location/LocationRegistry.java ---
@@ -49,35 +51,54 @@
/** removes the defined location from the registry (applications running there are unaffected) */
public void removeDefinedLocation(String id);
- /** returns fully populated (config etc) location from the given definition,
- * currently by creating it but TODO creation can be a leak so all current 'resolve' methods should be carefully used! */
+ /** Returns a fully populated (config etc) location from the given definition, with optional add'l flags.
+ * the location will be managed by default, unless the manage parameter is false or the CREATE_UNMANAGED flag is set.
+ * @since 0.7.0, but beta and likely to change as the semantics of this class are tuned */
+ @Beta
+ public Maybe<Location> resolve(LocationDefinition ld, Boolean manage, Map locationFlags);
--- End diff --
no, looking closer it is more confusing to have `true` be ignored. having `null` mean it will use flags or default is cleaner. leaving as is, but improving javadoc. (the whole API could be improved a lot of course.)
---
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.
---