Gilad Chaplik has posted comments on this change. Change subject: webadmin: introduce storage qos ......................................................................
Patch Set 6: (8 comments) new patch to follow http://gerrit.ovirt.org/#/c/29530/6//COMMIT_MSG Commit Message: Line 7: webadmin: introduce storage qos Line 8: Line 9: * storage qos subtab under datacenters. Line 10: * create/update/remove dialog storage qos. Line 11: * backend integration in GUI. > Yes, for each item mention the relevant section in the feature page. might be added later on to the feature page. I think that it's pretty straight fw, this patch adds a sub tab and relevant actions... :-) :-) Line 12: Line 13: Change-Id: I4d5c14409be9e1fad1df3487cf3615f4b4955930 http://gerrit.ovirt.org/#/c/29530/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/ConfigurationValues.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/ConfigurationValues.java: Line 140: DefaultMtu, Line 141: LiveMergeSupported(ConfigAuthType.User), Line 142: maxThroughputUpperBoundQosValue, Line 143: maxReadThroughputUpperBoundQosValue, Line 144: maxWriteThroughputUpperBoundQosValue, > are all of these hidden in the user portal? yes Line 145: maxIopsUpperBoundQosValue, Line 146: maxReadIopsUpperBoundQosValue, Line 147: maxWriteIopsUpperBoundQosValue; Line 148: http://gerrit.ovirt.org/#/c/29530/6/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/qos/DataCenterStorageQosListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/qos/DataCenterStorageQosListModel.java: Line 37: } Line 38: Line 39: @Override Line 40: protected RemoveQosModel<StorageQos> getRemoveQosModel() { Line 41: return new RemoveStorageQosModel(this); > use events instead.. since most of the UI (including network QoS) is implemented like that, that's not considered simple to me. we can later on discuss widely on how to do it in a better way... Line 42: } Line 43: Line 44: @Override Line 45: protected String getListName() { http://gerrit.ovirt.org/#/c/29530/6/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/qos/EditStorageQosModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/qos/EditStorageQosModel.java: Line 22: @Override Line 23: protected QosParametersBase<StorageQos> getParameters() { Line 24: QosParametersBase<StorageQos> qosParametersBase = new QosParametersBase<StorageQos>(); Line 25: qosParametersBase.setQos(getQos()); Line 26: qosParametersBase.setQosId(getQos().getId()); > why not extract the id from the object? removing setting the id. Line 27: return qosParametersBase; Line 28: } Line 29: Line 30: @Override http://gerrit.ovirt.org/#/c/29530/6/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/qos/RemoveQosModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/qos/RemoveQosModel.java: Line 87: setItems(list); Line 88: } else { Line 89: setMessage(getRemoveQosMessage(profiles.size())); Line 90: Line 91: ArrayList<String> list = new ArrayList<String>(); > then use object or something.. these two pieces of code are too similar for I've considered it, imo, there's no need to enforce such a simple code to an object. Line 92: for (ProfileBase item : profiles) { Line 93: list.add(item.getName()); Line 94: } Line 95: setItems(list); http://gerrit.ovirt.org/#/c/29530/6/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/qos/StorageQosMetricParametersModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/qos/StorageQosMetricParametersModel.java: Line 69: if (!getEnabled().getEntity()) { Line 70: return true; Line 71: } Line 72: Line 73: getTotal().validateEntity(new IValidation[] { > why not? a refactor here can mean less code, hence, less bugs :) Done :-) Line 74: new NotEmptyValidation(), Line 75: new IntegerValidation(0, Line 76: (Integer) AsyncDataProvider.getInstance().getConfigValuePreConverted(maxTotal)) }); Line 77: getRead().validateEntity(new IValidation[] { http://gerrit.ovirt.org/#/c/29530/6/frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIMessages.java File frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIMessages.java: Line 303: Line 304: @DefaultMessage("This Network QoS is used by {0} Vnic Profiles.\nAre you sure you want to remove this Network QoS?\n\n Profiles using this QoS:\n") Line 305: String removeNetworkQoSMessage(int numOfProfiles); Line 306: Line 307: @DefaultMessage("This Storage QoS is used by {0} Disk Profiles.\nAre you sure you want to remove this Storage QoS?\n\n Profiles using this QoS:\n") > i mean, why 'Profiles using this QoS:' is needed as apart of this string because I'm showing a list of profiles using that QoS object. Line 308: String removeStorageQoSMessage(int numOfProfiles); Line 309: Line 310: @DefaultMessage("{0} ({1} Socket(s), {2} Core(s) per Socket)") Line 311: String cpuInfoMessage(int numOfCpus, int sockets, int coresPerSocket); http://gerrit.ovirt.org/#/c/29530/6/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/qos/StorageQosWidget.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/qos/StorageQosWidget.java: Line 85: @WithElementId Line 86: IntegerEntityModelTextBoxOnlyEditor iopsWriteEditor; Line 87: Line 88: private StorageQosParametersModel model; Line 89: private final IEventListener availabilityListener; > didn't you create a PresenterWidget? I can do it in a later patch; I'm afraid this change is too risky at this stage. Line 90: Line 91: public StorageQosWidget(ApplicationConstants constants) { Line 92: throughputEnabled = new EntityModelCheckBoxEditor(Align.RIGHT); Line 93: iopsEnabled = new EntityModelCheckBoxEditor(Align.RIGHT); -- To view, visit http://gerrit.ovirt.org/29530 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4d5c14409be9e1fad1df3487cf3615f4b4955930 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
