Moti Asayag has posted comments on this change. Change subject: core: [RFE]Backup Awareness - DAO ......................................................................
Patch Set 1: Code-Review-1 (8 comments) https://gerrit.ovirt.org/#/c/39525/1/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/DbFacade.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/DbFacade.java: Line 1274: please use formatter. https://gerrit.ovirt.org/#/c/39525/1/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/EngineBackupHistoryDAO.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/EngineBackupHistoryDAO.java: Line 1: package org.ovirt.engine.core.dao; Line 2: Line 3: import org.ovirt.engine.core.common.businessentities.EngineBackupHistory; Line 4: Line 5: public interface EngineBackupHistoryDAO extends DAO { s/EngineBackupHistoryDAO/EngineBackupLogDao (also maintain camelCase) Line 6: Line 7: /** Line 8: * Gets the last successful engine backup record Line 9: */ https://gerrit.ovirt.org/#/c/39525/1/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/EngineBackupHistoryDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/EngineBackupHistoryDAODbFacadeImpl.java: Line 9: import java.sql.ResultSet; Line 10: import java.sql.SQLException; Line 11: Line 12: Line 13: public class EngineBackupHistoryDAODbFacadeImpl extends BaseDAODbFacade implements EngineBackupHistoryDAO { please annotate the class with: @Named @Singleton Which allow injecting this dao without any dependency on the DbFacade. Line 14: Line 15: private static class EngineBackupHistoryRowMapper implements RowMapper<EngineBackupHistory> { Line 16: public static final EngineBackupHistoryRowMapper instance = new EngineBackupHistoryRowMapper(); Line 17: Line 16: instance s/instance/INSTANCE ? (since it is static final...) Line 25: return entity; Line 26: } Line 27: } Line 28: Line 29: private class EngineBackupHistorySqlParameterSource extends CustomMapSqlParameterSource { this class isn't required. instead of using instances of it, please replace with: getCustomMapSqlParameterSource().addValue("db_name", dbName); Line 30: Line 31: public EngineBackupHistorySqlParameterSource(String dbName) { Line 32: super(getDialect()); Line 33: addValue("db_name", dbName); https://gerrit.ovirt.org/#/c/39525/1/backend/manager/modules/dal/src/main/jdbc-resources/engine-daos.properties File backend/manager/modules/dal/src/main/jdbc-resources/engine-daos.properties: Line 97: FenceAgentDAO=org.ovirt.engine.core.dao.FenceAgentDaoDbFacadeImpl Line 98: HostDeviceDao=org.ovirt.engine.core.dao.HostDeviceDaoDbFacadeImpl Line 99: StorageDeviceDao=org.ovirt.engine.core.dao.gluster.StorageDeviceDaoDbFacadeImpl Line 100: HostNicVfsConfigDao=org.ovirt.engine.core.dao.network.HostNicVfsConfigDaoDbFacadeImpl Line 101: EngineBackupHistoryDAO=org.ovirt.engine.core.dao.EngineBackupHistoryDAODbFacadeImpl > Have no idea why all marked as changed, only last line is new , will fix in because this file was removed and required any more. With latest patch regarding 'DAO injection' this is obsolete. https://gerrit.ovirt.org/#/c/39525/1/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/AuditLogDAOTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/AuditLogDAOTest.java: the new stored procedure isn't covered by a test: DeleteAuditNoOrOldBackupAlert. However it is probably not being used by the application directly. And yet it should be covered to reduce the case on which someone attempt to modify the table or column name. I don't have any good alternative but to add a method on DAO for it to make sure it runs fine. Line 1: package org.ovirt.engine.core.dao; Line 2: Line 3: import static org.junit.Assert.assertEquals; Line 4: import static org.junit.Assert.assertNotNull; https://gerrit.ovirt.org/#/c/39525/1/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/EngineBackupHistoryDAOTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/EngineBackupHistoryDAOTest.java: https://gerrit.ovirt.org/#/c/39523/1/packaging/dbscripts/engine_backup_history_sp.sql has 3 stored procedure. I think we should cover the other 2 as well in this file. Line 1: package org.ovirt.engine.core.dao; Line 2: Line 3: import org.junit.Before; Line 4: import org.junit.Test; -- To view, visit https://gerrit.ovirt.org/39525 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9b016e050f732eecb7f2872991d9fa9d31d9c024 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eli Mesika <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Martin Peřina <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Oved Ourfali <[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
