Livnat Peer has posted comments on this change. Change subject: core: Task Manager Add business entities ......................................................................
Patch Set 5: (4 inline comments) 2 general comments for this patch - 1. I would use long instead of Date for the different properties (strat time, end time, last update etc) mostly because of performance 2. I would not add the correlation Id logic in the setter. .................................................... File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/job/Job.java Line 71: why not use long (milliseconds) Line 157: } I don't think we should manipulate the correlation Id. I would add input validator for validating the correlation Id length. Line 223: lastUpdateTime = new Date(); why not use long and System current time? .................................................... File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/job/Step.java Line 191: } same comment about correlationId, no reason to have the substring logic -- To view, visit http://gerrit.ovirt.org/1085 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idc2351d2bb35cbad641f7ea0be93252602685ab0 Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Moti Asayag <[email protected]> Gerrit-Reviewer: Livnat Peer <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Michael Kublin <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
