Yair Zaslavsky has posted comments on this change. Change subject: core : Introduce engine_sessions table and DAO ......................................................................
Patch Set 5: (4 comments) http://gerrit.ovirt.org/#/c/35148/5/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/EngineSession.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/EngineSession.java: Line 11: Line 12: public class EngineSession implements Serializable { Line 13: private static final long serialVersionUID = 6964615561527013329L; Line 14: Line 15: private long id; Don't you want to move the changes you did in the next patch to this one ? (i.e - include a member of dbUser) Line 16: Line 17: private String engineSessionId; Line 18: Line 19: private Guid userId; Line 58: public Collection<Guid> getRoleIds() { Line 59: if (roleIds == null) { Line 60: roleIds = Collections.emptyList(); Line 61: } Line 62: return new ArrayList<Guid>(roleIds); As written before - not that critical but consider set (just to ensure nobody will be making a mistake of adding twice the same UUID - i mean, if they do add , you still should have only one copy). Line 63: } Line 64: Line 65: public void setRoleIds(Collection<Guid> roleIds) { Line 66: this.roleIds = roleIds; http://gerrit.ovirt.org/#/c/35148/5/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/EngineSessionDAOTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/EngineSessionDAOTest.java: Line 33: // create some test data Line 34: newEngineSession = new EngineSession(); Line 35: newEngineSession.setEngineSessionId(Guid.newGuid().toString()); Line 36: newEngineSession.setUserId(Guid.newGuid()); Line 37: newEngineSession.setRoleIds(new LinkedList<Guid>(Arrays.asList(FixturesTool.EXISTING_GROUP_ID))); not that it's that critical - but why not HashSet? Line 38: newEngineSession.setGroupIds(new LinkedList<Guid>(Arrays.asList(FixturesTool.EXISTING_GROUP_ID))); Line 39: newEngineSession.setUserName(""); Line 40: Line 41: existingEngineSession = dao.get(ID); http://gerrit.ovirt.org/#/c/35148/5/packaging/dbscripts/upgrade/03_06_0560_add_engine_sessions.sql File packaging/dbscripts/upgrade/03_06_0560_add_engine_sessions.sql: Line 8: group_ids text, Line 9: role_ids text, Line 10: CONSTRAINT pk_engine_session PRIMARY KEY (id) Line 11: ); Line 12: CREATE INDEX idx_engine_session_session_id ON engine_sessions USING btree (engine_session_id); What about foreign key to users table? -- To view, visit http://gerrit.ovirt.org/35148 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4b4d9cfc3edc6084fc0436ecfd09c82d5ae57f5e Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Ravi Nori <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[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
