Laszlo Hornyak has posted comments on this change. Change subject: core: Remove GetVdsHooksById2Query ......................................................................
Patch Set 4: Looks good to me, but someone else must approve (2 inline comments) looks good in general, just some minor comment inline .................................................... File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/vdshooks/VdsHooksParser.java Line 152 Line 153 Line 154 Line 155 Line 156 wondering if this notEmpty just an optimization or a protection from an NPE. vds_dynamic hooks is not marked as not null thefore VDS.getHookStr() could return null Probably it would be safer to keep the null protection just for the case? .................................................... File backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/vdshooks/TestVdsHooks.java Line 57 Line 58 Line 59 Line 60 Line 61 Hi Allon, Kind of obfuscated unit test, but probably it could be saved and rewritten to the remaining parseHooks method. And then it could be called VdsHooksParserTest. What is your opinion? -- To view, visit http://gerrit.ovirt.org/13607 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1890f429a12cbf7e42727b7360a85ebb1b07b5b0 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Allon Mureinik <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Laszlo Hornyak <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
