brooklyncentral / brooklyn

This project has moved and is now part of the ASF
https://github.com/apache/incubator-brooklyn
72 stars 27 forks source link

Fix: MultiLocation leak #1405

Closed aledsage closed 10 years ago

aledsage commented 10 years ago

And some other minor improvements.

buildhive commented 10 years ago

Brooklyn Central » brooklyn #2302 FAILURE Looks like there's a problem with this pull request (what's this?)

grkvlt commented 10 years ago

@aledsage Looks reasonable, probably just need to move the static methods to Locations depends on what you thing of the isChildOf predicate comment?

aledsage commented 10 years ago

@grkvlt have incorporated your comments. Was in two minds about moving Entities.isManaged(location) to Locations. It's nice that it's next to Entities.isManaged(entity) but overall I agree with you so have moved it. Can you take another look and merge if you agree.

buildhive commented 10 years ago

Brooklyn Central » brooklyn #2333 SUCCESS This pull request looks good (what's this?)

grkvlt commented 10 years ago

@aledsage All good except I'm not sure about the LocationPredicates#isDescendantOf(Location) implementation, specifically the or equal to part of the check. It would be simple to change to remove this check, but wasn't sure if you had a specific use-case in mind?

grkvlt commented 10 years ago

@aledsage Just noticed a few @Nullable annotations in the location predicates that need dealt with as well

richardcloudsoft commented 10 years ago

@grkvlt, being @Nullable is part of the Predicate interface - same is true for Functions too - so probably not strictly correct to remove the annotation. @aledsage I always put an if(input == null) return false as a matter of routine.

grkvlt commented 10 years ago

@aledsage Agree with @richardcloudsoft, have to add the null checking guard, I'd forgotten the annotation was specified in the Predicate interface

aledsage commented 10 years ago

Thanks @grkvlt @richardcloudsoft - have incorporated those comments (including for some of our other *Predicates.java classes. Merging.

buildhive commented 10 years ago

Brooklyn Central » brooklyn #2340 FAILURE Looks like there's a problem with this pull request (what's this?)

grkvlt commented 10 years ago

brooklyn.rest.resources.ApplicationResourceTest.testDeleteApplication (from TestSuite)

Failing for the past 2 builds (Since Failed#2339 ) Took 0.12 sec.

Error Message

apps=[BasicApplicationImpl{id=XmK14eSg}, RestMockApp{id=E3363uM8}, BasicApplicationImpl{id=W0Ys0loG}, BasicApplicationImpl{id=qlP28Cby}] expected [3] but found [4]

Stacktrace

java.lang.AssertionError: apps=[BasicApplicationImpl{id=XmK14eSg}, RestMockApp{id=E3363uM8}, BasicApplicationImpl{id=W0Ys0loG}, BasicApplicationImpl{id=qlP28Cby}] expected [3] but found [4] at org.testng.Assert.fail(Assert.java:94) at org.testng.Assert.failNotEquals(Assert.java:494) at org.testng.Assert.assertEquals(Assert.java:123) at org.testng.Assert.assertEquals(Assert.java:370) at brooklyn.rest.resources.ApplicationResourceTest.testDeleteApplication(ApplicationResourceTest.java:493)