ISPN-1154, ISPN-1167, ISPN-1166 #367
Conversation
code change is one, hence a single commit but two issues to track them: ISPN-1167 Eviction acquires locks even if it doesn't need them ISPN-1166 Eviction might release locks it didn't acquire
| * | ||
| * @author Sanne Grinovero <sanne@infinispan.org> (C) 2011 Red Hat Inc. | ||
| */ | ||
| public final class ImmutableContext implements InvocationContext { |
There was a problem hiding this comment.
If you want to create a singleton, using an enum is more elegant, for example: http://stackoverflow.com/questions/70689/efficient-way-to-implement-singleton-pattern-in-java
There was a problem hiding this comment.
@galderz, it seems that converting it to an enum could lead to some trouble: "clone()" on enum is declared final, and throws unsupported operation, while Infinispan redefines the clone() on the interface. it seems we could convert it to an enum nevertheless as it looks like clone() would not be invoked on the cases this implementation is used, but it seems to me that we would be safer keeping the patch as-is. WDYT ?
There was a problem hiding this comment.
ah, actually it's just not possible to do it, as the return type of an enum is different than what our signature requires. So we're stuck with the singleton constant.
Anyway, it seems the beautiful pattern of the enum is mostly useful to make sure there's really only one instance (including serialized clones), but we don't really need strong guarantees on it.
There was a problem hiding this comment.
That's fine Sanne. It's always worth looking an alternatives. Great job investigating it.
|
p.s. If you add links to the JIRAs, it makes it easier for the reviewer :) |
|
Btw, just to clarify as per the dev thread, there's still a change missing where the passivaition will occur before the eviction, right? |
|
hi @galderz, many thanks for the review. Yes this is not including a pull request for https://issues.jboss.org/browse/ISPN-1169 , as that will require some more discussing and a lot of testing so I wanted to get this in as a stable starting point, as these issues need to be solved too. |
(are all related)