Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/867#discussion_r147279623
  
    --- Diff: 
core/src/main/java/org/apache/brooklyn/core/typereg/BasicManagedBundle.java ---
    @@ -135,6 +145,9 @@ public boolean equals(Object obj) {
                 // this makes equality with other OsgiBundleWithUrl items 
symmetric,
                 // but for two MB's we look additionally at checksum
                 if (!Objects.equal(checksum, 
((ManagedBundle)other).getChecksum())) return false;
    +            
    +            // only equal if have the same ManagedBundle uid; important 
for persistence.changeListener().unmanage()
    +            if (!Objects.equal(getId(), ((ManagedBundle)other).getId())) 
return false;
    --- End diff --
    
    I don't like this: it breaks transitivity for the equals method: if 
`x.equals(y) && `y.equals(z)` then `x.equals(z)` should be true. It fails if 
`x` and `z` are `BasicManagedBundle` with different checksums or ids, but `y` 
is a `OsgiBundleWithUrl` with the same name:version and url.
    
    However, that was already broken because of the checksum comparison (if an 
`OsgiBundleWithUrl` impl behaves in the way indicated in the pre-existing 
comments).
    
    Longer term, I lean towards not trying to have equality with 
`OsgiBundleWithUrl`.


---

Reply via email to