Yair Zaslavsky has posted comments on this change. Change subject: core: Refactoring the CreateVDSCommand ......................................................................
Patch Set 1: (4 inline comments) .................................................... File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/CreateVDSCommand.java Line 36: protected VM mVm; if you did some styling to Java (in the logCommandInfo) for example - can you please change mVm to something else? :) Line 73: buildVmProperties(); Thank you for the separation to methods here! :) Line 87: mCreateInfo.add(VdsProperties.num_of_monitors, can you remove this empty line? there are other places later on in this file, where I think it should be done as well (I think that when you are in the middle of arguments for method, it's ok to go to new lines, but dont insert empty lines in the middle). Line 303: Pattern regexPattern = Pattern.compile(String.format(OS_REGEX, vmOS)); If we have a fixed set of supported OS, can we maybe hold in a static map a repository of compiled expressions, and perform compilation upon backend startup? This can help performance later on when running the "create" verb. -- To view, visit http://gerrit.ovirt.org/1151 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7a0d4a4dfc868775a7e31c6bc6c7f3eae2ab2ee3 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
