Lior Vernia has posted comments on this change. Change subject: core: Added DAO for HostNetworkQos entities ......................................................................
Patch Set 1: (4 comments) http://gerrit.ovirt.org/#/c/34121/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/QosQueryBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/QosQueryBase.java: Line 6: import org.ovirt.engine.core.dao.qos.QosDao; Line 7: Line 8: Line 9: public abstract class QosQueryBase extends QueriesCommandBase<QosQueryParameterBase> { Line 10: private QosDao<?> qosDao; > what's the point of this field? It seems like caching (which is inherently I'll submit a succeeding refactoring. Line 11: Line 12: public QosQueryBase(QosQueryParameterBase parameters) { Line 13: super(parameters); Line 14: } Line 17: QosType qosType = getParameters().getQosType(); Line 18: if (qosType == null) { Line 19: return getDbFacade().getQosBaseDao(); Line 20: } Line 21: switch (qosType) { > I'd create QosDaoRegistrar class so every Dao would register itself with th See previous comment. Line 22: case STORAGE: Line 23: qosDao = getDbFacade().getStorageQosDao(); Line 24: break; Line 25: case CPU: Line 30: break; Line 31: case HOSTNETWORK: Line 32: qosDao = getDbFacade().getHostNetworkQosDao(); Line 33: break; Line 34: default: > notice that in default case null will be returned causing NPE in all usages See previous comment. Line 35: log.debugFormat("Not handled QoS type: {0}", qosType); Line 36: break; Line 37: } Line 38: return qosDao; http://gerrit.ovirt.org/#/c/34121/1/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/qos/AllQosBaseDaoFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/qos/AllQosBaseDaoFacadeImpl.java: Line 54: case NETWORK: Line 55: return NetworkQoSDaoFacadeImpl.NetworkQosDaoDbFacadaeImplMapper.MAPPER.createQosEntity(rs); Line 56: case HOSTNETWORK: Line 57: return HostNetworkQosDaoDbFacadeImpl.HostNetworkQosDaoDbFacadaeImplMapper.MAPPER.createQosEntity(rs); Line 58: default: > there is code style issue: once we're immediately returning from case, once Belongs to a different patch, will post and put you as a reviewer. Line 59: log.debugFormat("not handled/missing qos_type", qosType); Line 60: break; Line 61: } Line 62: -- To view, visit http://gerrit.ovirt.org/34121 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I90535167d324c80aa9de7a735a010818ab45e428 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Lior Vernia <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Yevgeny Zaspitsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
