Juan Hernandez has posted comments on this change.
Change subject: Trusted Compute Pools - Open Attestation integration with oVirt
engine proposal
......................................................................
Patch Set 3: (12 inline comments)
Thanks for the change to use commons-httpclient.
I have some additional comments about the use of the singletons. See the inline
comments.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/attestationbroker/AttestationCacheManager.java
Line 8: public class AttestationCacheManager {
Line 9:
Line 10: private static final AttestationCacheManager instance = new
AttestationCacheManager();
Line 11: //TODO Consider clearing cache when there are no softreference?
Line 12: private static ConcurrentHashMap<String, AttestationValue>
attestationValues = new ConcurrentHashMap<String, AttestationValue>();
As you are going to use this as a singleton the members and methods shouldn't
be static.
Line 13: private static long cacheTimeout = Long.valueOf(Config.<String>
GetValue(ConfigValues.CacheTimeout)).longValue();
Line 14:
Line 15: public AttestationCacheManager getInstance(){
Line 16: return instance;
Line 24: return attestationValues.containsKey(key);
Line 25: }
Line 26:
Line 27: public static synchronized void addToCache(AttestationValue value){
Line 28: if (!attestationValues.containsKey(value.getHostName()))
This "if" is not needed, the "putIfAbsent" method already performs the check,
and it doees it in a atomic way, so the "synchronized" modifier is not required.
Line 29: attestationValues.putIfAbsent(value.getHostName(), value);
Line 30: }
Line 31:
Line 32: public static synchronized void removeFromCache(String key){
Line 28: if (!attestationValues.containsKey(value.getHostName()))
Line 29: attestationValues.putIfAbsent(value.getHostName(), value);
Line 30: }
Line 31:
Line 32: public static synchronized void removeFromCache(String key){
The remove method of the ConcurrentHashMap already performs its own
synchronization, so the "synchronized" modifier is not required.
Line 33: if (key != null){
Line 34: attestationValues.remove(key);
Line 35: }
Line 36: }
Line 52: public static Set<String> getAllKeys(){
Line 53: return attestationValues.keySet();
Line 54: }
Line 55:
Line 56: public static synchronized void updateCache(AttestationValue
value){
Instead of using the "synchronzed" modifier you can rely on the synchronization
of the ConcurrentHashMap and do something like this:
AttestationValue cacheValue = attestationValues.get(value.getHostName());
if (cacheValue != null) {
cacheValue.setTrustLeve(...);
cacheVaue.setVtime(...);
}
Line 57: if (exists(value.getHostName())){
Line 58: AttestationValue cacheValue =
getValueFromCache(value.getHostName());
Line 59: cacheValue.setTrustLevel(value.getTrustLevel());
Line 60: cacheValue.setVtime(value.getVtime());
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/attestationbroker/AttestationService.java
Line 69: public AttestationResult validateHostIsTrusted(VDS vds) {
Line 70: log.debug("begin to validate " + vds.getName());
Line 71: AttestationResult result = AttestationResult.UNKNOWN;
Line 72: String key = vds.gethost_name();
Line 73: if (AttestationCacheManager.exists(key)){
If you use the AttestationCacheManager as a singleton this should be:
AttestatoinCacheManager.getInstance().exists(key)
I would suggest to create a private field, to avoid these long invocations:
private AttestationCacheManager cacheManager =
AttestationCacheManager.getInstance();
if (cacheManager.exists(key)) {
...
if (cacheManager.isExpired()) {
...
}
}
This has the advantage that if one day we start using CDI and annotiations we
can convert these singletons into singletons managed by the container, simple
adding annotations.
Line 74: if (!AttestationCacheManager.isExpired(key)){
Line 75: AttestationValue value =
AttestationCacheManager.getValueFromCache(key);
Line 76: return value.getTrustLevel();
Line 77: }
Line 107: }
Line 108: }catch (UnsupportedEncodingException e) {
Line 109: log.error(e.getMessage(), e);
Line 110: }catch (IOException e) {
Line 111: log.error(e.getMessage(), e);
I would suggest to include here some message explaining what failed, not just
directly the message of the underlying exception:
log.error("Failed to attest hosts " + theListOfHosts, e);
Line 112: }finally {
Line 113: postMethod.releaseConnection();
Line 114: }
Line 115: return values;
Line 149: }
Line 150: break;
Line 151: }
Line 152: }
Line 153: jParser.close();
I would suggest to put this in a "finally" block.
Line 154: } catch (JsonParseException e) {
Line 155: log.error(e.getMessage(), e);
Line 156: } catch (IOException e) {
Line 157: log.error(e.getMessage(), e);
Line 153: jParser.close();
Line 154: } catch (JsonParseException e) {
Line 155: log.error(e.getMessage(), e);
Line 156: } catch (IOException e) {
Line 157: log.error(e.getMessage(), e);
I would suggest a more informative error message, something like:
"Failed to parse attestation response \"" + str + "\"."
Line 158: }
Line 159: return values;
Line 160: }
Line 161:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/attestationbroker/AttestationValue.java
Line 7:
Line 8: private String hostName;
Line 9: private AttestationResult trustLevel;
Line 10: // Real time of attestation
Line 11: private Date vtime;
Thanks for the comment! Can't it be in the variable name? Something like
"realAttestationTime". I like long variable names, it is just a personal
preference, feel free to ignore.
Line 12:
Line 13: public AttestationValue(){
Line 14: trustLevel = AttestationResult.TIMEOUT;
Line 15: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsSelector.java
Line 30: import org.ovirt.engine.core.dao.network.NetworkDao;
Line 31: import org.ovirt.engine.core.dao.network.VmNetworkInterfaceDao;
Line 32: import org.ovirt.engine.core.utils.log.Log;
Line 33: import org.ovirt.engine.core.utils.log.LogFactory;
Line 34: import org.ovirt.engine.core.bll.attestationbroker.*;
Use explicit class names instead of *, if possible.
Line 35:
Line 36: public class VdsSelector {
Line 37: private final List<Guid> privateRunVdssList = new
ArrayList<Guid>();
Line 38: private List<VmNetworkInterface> vmNICs;
Line 255: return
VdcBllMessages.ACTION_TYPE_FAILED_VDS_VM_CLUSTER;
Line 256: }
Line 257: return null;
Line 258: }
Line 259: });
This new validator is exactly the same that the one below, I guess this is a
result of automatic merging. Can you remove it?
Line 260: add(new HostValidator() {
Line 261:
Line 262: @Override
Line 263: public VdcBllMessages validate(VDS vds, StringBuilder
sb, boolean isMigrate) {
....................................................
File
backend/manager/modules/searchbackend/src/main/java/org/ovirt/engine/core/searchbackend/DateUtils.java
Line 8: import java.util.List;
Line 9:
Line 10: import org.ovirt.engine.core.compat.DateTime;
Line 11:
Line 12: public class DateUtils {
There is an ongoing effort to remove all the classes in the "compat" package.
Can you do without this class? Maybe use directly "java.text.DateFormat"?
Line 13:
Line 14: public static Date parse(String str) {
Line 15: for (DateFormat fmt : formats(DateFormat.DEFAULT,
DateFormat.FULL, DateFormat.LONG, DateFormat.MEDIUM, DateFormat.SHORT)) {
Line 16: try {
--
To view, visit http://gerrit.ovirt.org/11237
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4de780cd46069638433255f3f9c994575f752e55
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Dave Chen <[email protected]>
Gerrit-Reviewer: Dave Chen <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Itamar Heim <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
