Juan Hernandez has posted comments on this change.
Change subject: core: Cache JNDI lookups and InitialContext creation
......................................................................
Patch Set 1: (4 inline comments)
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ejb/EJBUtilsStrategy.java
Line 53:
Line 54: if (context == null)
Line 55: context = new InitialContext();
Line 56:
Line 57: return context;
You could use the double check with volatile idiom to avoid a synchronization
with every call to the method:
private static volatile InitialContext context;
private static InitialContext getInitialContext() throws NamingException {
if (context == null) {
synchronized(EJBUtilsStrategy.class) {
if (context == null) {
context = new InitialContext();
}
}
}
}
However synchronization is quite fast nowadays, so this is probably overkill.
Just consider it.
Line 58: }
Line 59:
Line 60: /**
Line 61: * Finds a bean according to its JNDI name
Line 106: jndiNameSB.append(jndiNameFromMap);
Line 107: jndiNameSB.append(getBeanSuffix(beanType, proxyType));
Line 108:
Line 109: if (proxyType == BeanProxyType.LOCAL) {
Line 110: // context = new InitialContext();
Don't leave this commented.
Line 111: context = getInitialContext();
Line 112: }
Line 113:
Line 114: // appends "local" or "remote" to the jndi name, depends
on the
Line 130: return null;
Line 131: }
Line 132: }
Line 133:
Line 134: private <T> T getReference(String refName) throws NamingException
{
I think that this method should be synchronized, that or use a concurrent hash
map, otherwise we get exposed to random bugs.
Line 135: if (cachedJNDIReferences.containsKey(refName)) {
Line 136: return (T) cachedJNDIReferences.get(refName);
Line 137: }
Line 138: Object reference = getInitialContext().lookup(refName);
Line 133:
Line 134: private <T> T getReference(String refName) throws NamingException
{
Line 135: if (cachedJNDIReferences.containsKey(refName)) {
Line 136: return (T) cachedJNDIReferences.get(refName);
Line 137: }
I would suggest to do as follows:
T reference = (T) cachedJNDIReferences.get(refName);
if (reference != null) {
return reference;
}
That way you do one call to the map instead of two, thus reducing contention
and chances of races.
Line 138: Object reference = getInitialContext().lookup(refName);
Line 139: cachedJNDIReferences.put(refName, reference);
Line 140:
Line 141: return (T) reference;
--
To view, visit http://gerrit.ovirt.org/16612
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I45ae8d0a90a03c498e19e5429d196924671c073b
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liran Zelkha <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
