Gilad Chaplik has posted comments on this change. Change subject: webadmin: introduce storage qos ......................................................................
Patch Set 6: (39 comments) 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. > please elaborate on each change if possible added a feature page link, is there anything in particular? Done. Line 12: Line 13: Change-Id: I4d5c14409be9e1fad1df3487cf3615f4b4955930 Line 8: Line 9: * storage qos subtab under datacenters. Line 10: * create/update/remove dialog storage qos. Line 11: * backend integration in GUI. Line 12: > add feature page url and preferably some screen-shots. screen shot will become avail in feature page. Done. 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 138: SpiceFileTransferToggleSupported(ConfigAuthType.User), Line 139: SpiceCopyPasteToggleSupported(ConfigAuthType.User), Line 140: DefaultMtu, Line 141: LiveMergeSupported(ConfigAuthType.User), Line 142: maxThroughputUpperBoundQosValue, > change first letter to upper case (same as the others key) Done Line 143: maxReadThroughputUpperBoundQosValue, Line 144: maxWriteThroughputUpperBoundQosValue, Line 145: maxIopsUpperBoundQosValue, Line 146: maxReadIopsUpperBoundQosValue, http://gerrit.ovirt.org/#/c/29530/6/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/qos/DataCenterQosListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/qos/DataCenterQosListModel.java: Line 77: @SuppressWarnings("unchecked") Line 78: @Override Line 79: public void onSuccess(Object model, Object returnValue) { Line 80: DataCenterQosListModel<T, P> qosListModel = (DataCenterQosListModel) model; Line 81: qosListModel.setItems((ArrayList<T>) ((VdcQueryReturnValue) returnValue).getReturnValue()); > just call 'setItems' prefer not to, since it's not the same object. I can call DataCenterQosListModel.this.setItems(..) or leave both above comments. Line 82: Line 83: } Line 84: }; Line 85: IdQueryParameters parameters = new QosQueryParameterBase(getEntity().getId(), getQosType()); Line 92: @Override Line 93: protected void entityPropertyChanged(Object sender, PropertyChangedEventArgs e) { Line 94: super.entityPropertyChanged(sender, e); Line 95: Line 96: if (e.propertyName.equals("name")) { //$NON-NLS-1$ > why is it needed? just in case name changed? Done. Line 97: getSearchCommand().execute(); Line 98: } Line 99: } Line 100: Line 140: if (getConfirmWindow() != null) { Line 141: return; Line 142: } Line 143: Line 144: ConfirmationModel model = getRemoveQosModel(); > variable can be removed Done Line 145: setConfirmWindow(model); Line 146: } Line 147: Line 148: public void edit() { Line 145: setConfirmWindow(model); Line 146: } Line 147: Line 148: public void edit() { Line 149: final T qos = (T) getSelectedItem(); > why final? Done Line 150: Line 151: if (getWindow() != null) { Line 152: return; Line 153: } Line 154: Line 155: final EditQosModel<T, P> qosModel = getEditQosModel(qos); Line 156: setWindow(qosModel); Line 157: Line 158: qosModel.getDataCenters().setItems(Arrays.asList(getEntity())); > use this signature: 'setItems(Collection<T> value, T selectedItem)' Done Line 159: qosModel.getDataCenters().setSelectedItem(getEntity()); Line 160: Line 161: } Line 162: Line 160: Line 161: } Line 162: Line 163: @Override Line 164: protected void onSelectedItemChanged() { > why override the method? Done Line 165: super.onSelectedItemChanged(); Line 166: updateActionAvailability(); Line 167: } Line 168: Line 166: updateActionAvailability(); Line 167: } Line 168: Line 169: @Override Line 170: protected void selectedItemsChanged() { > same Done Line 171: super.selectedItemsChanged(); Line 172: updateActionAvailability(); Line 173: } Line 174: Line 186: Line 187: final NewQosModel<T, P> newQosModel = getNewQosModel(); Line 188: setWindow(newQosModel); Line 189: Line 190: newQosModel.getDataCenters().setItems(Arrays.asList(getEntity())); > use this signature: 'setItems(Collection<T> value, T selectedItem)' Done Line 191: newQosModel.getDataCenters().setSelectedItem(getEntity()); Line 192: } Line 193: 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 5: import org.ovirt.engine.ui.uicommonweb.help.HelpTag; Line 6: import org.ovirt.engine.ui.uicompat.ConstantsManager; Line 7: Line 8: public class DataCenterStorageQosListModel extends DataCenterQosListModel<StorageQos, StorageQosParametersModel> { Line 9: @Override > formatter what is not formatted? Line 10: protected String getQosTitle() { Line 11: return ConstantsManager.getInstance().getConstants().storageQosTitle(); Line 12: } Line 13: Line 32: } Line 33: Line 34: @Override Line 35: protected EditQosModel<StorageQos, StorageQosParametersModel> getEditQosModel(StorageQos qoS) { Line 36: return new EditStorageQosModel((StorageQos) getSelectedItem(), this, getEntity()); > is casting needed? (doesn't getSelectedItem use generics?) apparently not. Line 37: } Line 38: Line 39: @Override Line 40: protected RemoveQosModel<StorageQos> getRemoveQosModel() { Line 37: } Line 38: Line 39: @Override Line 40: protected RemoveQosModel<StorageQos> getRemoveQosModel() { Line 41: return new RemoveStorageQosModel(this); > is it really mandatory to pass the entire listmodel into the model?! i don' do you have a simple alternative? 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 passing both object and id? a base parameter class for all qos objects, used for all CRUD commands, requires both id and obejct. 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/QosModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/qos/QosModel.java: Line 43: return getIsValid(); Line 44: } Line 45: Line 46: protected void addCommands() { Line 47: UICommand tempVar2 = new UICommand("OnSave", this); //$NON-NLS-1$ > rename tempvar Done Line 48: tempVar2.setTitle(ConstantsManager.getInstance().getConstants().ok()); Line 49: tempVar2.setIsDefault(true); Line 50: getCommands().add(tempVar2); Line 51: UICommand tempVar3 = new UICommand("Cancel", this); //$NON-NLS-1$ Line 47: UICommand tempVar2 = new UICommand("OnSave", this); //$NON-NLS-1$ Line 48: tempVar2.setTitle(ConstantsManager.getInstance().getConstants().ok()); Line 49: tempVar2.setIsDefault(true); Line 50: getCommands().add(tempVar2); Line 51: UICommand tempVar3 = new UICommand("Cancel", this); //$NON-NLS-1$ > same Done Line 52: tempVar3.setTitle(ConstantsManager.getInstance().getConstants().cancel()); Line 53: tempVar3.setIsCancel(true); Line 54: getCommands().add(tempVar3); Line 55: } Line 70: Line 71: public abstract void init(T qos); Line 72: Line 73: @Override Line 74: public abstract String getTitle(); > why override? it's only virtual in parent, and I like to enforce it. Line 75: Line 76: public abstract HelpTag getHashTag(); Line 77: Line 78: @Override Line 75: Line 76: public abstract HelpTag getHashTag(); Line 77: Line 78: @Override Line 79: public abstract String getHashName(); > why override? same Line 80: Line 81: protected void cancel() { Line 82: sourceModel.setWindow(null); Line 83: sourceModel.setConfirmWindow(null); http://gerrit.ovirt.org/#/c/29530/6/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/qos/QosParametersModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/qos/QosParametersModel.java: Line 6: public abstract class QosParametersModel<T extends QosBase> extends Model { Line 7: Line 8: public abstract void init(T qos); Line 9: Line 10: public abstract void flush(T qos); > add empty line Done 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 1: package org.ovirt.engine.ui.uicommonweb.models.datacenters.qos; Line 2: Line 3: > remove empty line Done Line 4: import java.util.ArrayList; Line 5: import java.util.List; Line 6: Line 7: import org.ovirt.engine.core.common.action.QosParametersBase; Line 34: addCommands(); Line 35: } Line 36: Line 37: private void addCommands() { Line 38: UICommand tempVar = new UICommand("onRemove", this); //$NON-NLS-1$ > rename tempVar Done Line 39: tempVar.setTitle(ConstantsManager.getInstance().getConstants().ok()); Line 40: tempVar.setIsDefault(true); Line 41: getCommands().add(tempVar); Line 42: UICommand tempVar2 = new UICommand("cancel", this); //$NON-NLS-1$ Line 38: UICommand tempVar = new UICommand("onRemove", this); //$NON-NLS-1$ Line 39: tempVar.setTitle(ConstantsManager.getInstance().getConstants().ok()); Line 40: tempVar.setIsDefault(true); Line 41: getCommands().add(tempVar); Line 42: UICommand tempVar2 = new UICommand("cancel", this); //$NON-NLS-1$ > rename tempVar Done Line 43: tempVar2.setTitle(ConstantsManager.getInstance().getConstants().cancel()); Line 44: tempVar2.setIsCancel(true); Line 45: getCommands().add(tempVar2); Line 46: } Line 58: Line 59: protected abstract VdcActionType getRemoveActionType(); Line 60: Line 61: private void setMessage() { Line 62: > remove line Done Line 63: ArrayList<VdcQueryParametersBase> parameters = new ArrayList<VdcQueryParametersBase>(); Line 64: ArrayList<VdcQueryType> queryTypes = new ArrayList<VdcQueryType>(); Line 65: for (T qos : sourceListModel.getSelectedItems()) { Line 66: VdcQueryParametersBase parameter = new IdQueryParameters(qos.getId()); Line 67: parameters.add(parameter); Line 68: queryTypes.add(getProfilesByQosIdQueryType()); Line 69: } Line 70: Frontend.getInstance().runMultipleQueries(queryTypes, parameters, new IFrontendMultipleQueryAsyncCallback() { Line 71: > remove line prefer to leave a line. Line 72: @Override Line 73: public void executed(FrontendMultipleQueryAsyncResult result) { Line 74: List<ProfileBase> profiles = new ArrayList<ProfileBase>(); Line 75: Line 87: setItems(list); Line 88: } else { Line 89: setMessage(getRemoveQosMessage(profiles.size())); Line 90: Line 91: ArrayList<String> list = new ArrayList<String>(); > reuse here cannot reuse. two different lists (one of type profile base and the other is qos) 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 14: private EntityModel<Integer> total; Line 15: private EntityModel<Integer> read; Line 16: private EntityModel<Integer> write; Line 17: private EntityModel<Boolean> enabled; Line 18: private final ConfigurationValues maxTotal; > why final? since it's set in c'tor. Line 19: private final ConfigurationValues maxRead; Line 20: private final ConfigurationValues maxWrite; Line 21: Line 22: public StorageQosMetricParametersModel(ConfigurationValues maxTotal, Line 69: if (!getEnabled().getEntity()) { Line 70: return true; Line 71: } Line 72: Line 73: getTotal().validateEntity(new IValidation[] { > can you consolidate these three sections (extract to a method, etc..) I prefer to have a single method dedicated for validation, don't see any benefit in such a refactor. 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") > redundant posfix? what? 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/ApplicationConstants.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/ApplicationConstants.java: Line 2776: // Disk Line 2777: @DefaultStringValue("ID") Line 2778: String idDisk(); Line 2779: Line 2780: @Override > ? looks like it's overriding base, and added by formatter, I guess that we should remove this entry from the class. Line 2781: @DefaultStringValue("Quota") Line 2782: String quotaDisk(); Line 2783: Line 2784: @DefaultStringValue("Volume Format") http://gerrit.ovirt.org/#/c/29530/6/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/gin/PresenterModule.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/gin/PresenterModule.java: Line 1345: bindPresenterWidget(NetworkQoSPopupPresenterWidget.class, Line 1346: NetworkQoSPopupPresenterWidget.ViewDef.class, Line 1347: NetworkQoSPopupView.class); Line 1348: Line 1349: // Storage qos > s/qos/QoS Done Line 1350: bindPresenterWidget(StorageQosPopupPresenterWidget.class, Line 1351: StorageQosPopupPresenterWidget.ViewDef.class, Line 1352: StorageQosPopupView.class); Line 1353: http://gerrit.ovirt.org/#/c/29530/6/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/tab/datacenter/SubTabDataCenterStorageQosPresenter.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/tab/datacenter/SubTabDataCenterStorageQosPresenter.java: Line 37: Line 38: @TabInfo(container = DataCenterSubTabPanelPresenter.class) Line 39: static TabData getTabData(ApplicationConstants applicationConstants, Line 40: SearchableDetailModelProvider<StorageQos, DataCenterListModel, DataCenterStorageQosListModel> modelProvider) { Line 41: return new ModelBoundTabData(applicationConstants.dataCenterStorageQosSubTabLabel(), 10, modelProvider); > why 10? shouldn't it be near network QOS? it's going to be move in next patch. I think I've told you about it several times already. Line 42: } Line 43: Line 44: @Inject Line 45: public SubTabDataCenterStorageQosPresenter(EventBus eventBus, ViewDef view, ProxyDef proxy, http://gerrit.ovirt.org/#/c/29530/6/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/qos/StorageQosPopupView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/qos/StorageQosPopupView.java: Line 87: Line 88: @Override Line 89: public void edit(QosModel<StorageQos, StorageQosParametersModel> object) { Line 90: qosWidget.edit(object.getQosParametersModel()); Line 91: driver.edit(object); > call driver's edit first Done Line 92: } Line 93: Line 94: @Override Line 95: public QosModel<StorageQos, StorageQosParametersModel> flush() { http://gerrit.ovirt.org/#/c/29530/6/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/qos/StorageQosPopupView.ui.xml File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/qos/StorageQosPopupView.ui.xml: Line 2: <!DOCTYPE ui:UiBinder SYSTEM "http://dl.google.com/gwt/DTD/xhtml.ent";> Line 3: <ui:UiBinder xmlns:ui="urn:ui:com.google.gwt.uibinder" Line 4: xmlns:g="urn:import:com.google.gwt.user.client.ui" xmlns:d="urn:import:org.ovirt.engine.ui.common.widget.dialog" Line 5: xmlns:e="urn:import:org.ovirt.engine.ui.common.widget.editor" xmlns:ge="urn:import:org.ovirt.engine.ui.common.widget.editor.generic" Line 6: xmlns:n="urn:import:org.ovirt.engine.ui.webadmin.section.main.view.popup.qos"> > s/xmlns:n/xmlns:q Done Line 7: Line 8: <ui:style> Line 9: .topDecorator { Line 10: background-color: #D3D3D3; 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 84: @Path(value = "iops.write.entity") Line 85: @WithElementId Line 86: IntegerEntityModelTextBoxOnlyEditor iopsWriteEditor; Line 87: Line 88: private StorageQosParametersModel model; > redundant used by toggleVisibility Line 89: private final IEventListener availabilityListener; Line 90: Line 91: public StorageQosWidget(ApplicationConstants constants) { Line 92: throughputEnabled = new EntityModelCheckBoxEditor(Align.RIGHT); Line 85: @WithElementId Line 86: IntegerEntityModelTextBoxOnlyEditor iopsWriteEditor; Line 87: Line 88: private StorageQosParametersModel model; Line 89: private final IEventListener availabilityListener; > move to the presenter which presenter? it's a widget. Line 90: Line 91: public StorageQosWidget(ApplicationConstants constants) { Line 92: throughputEnabled = new EntityModelCheckBoxEditor(Align.RIGHT); Line 93: iopsEnabled = new EntityModelCheckBoxEditor(Align.RIGHT); Line 136: @Override Line 137: public void edit(StorageQosParametersModel model) { Line 138: driver.edit(model); Line 139: Line 140: if (this.model != null) { > move listener registration to the presenter same. Line 141: this.model.getPropertyChangedEvent().removeListener(availabilityListener); Line 142: } Line 143: this.model = model; Line 144: model.getPropertyChangedEvent().addListener(availabilityListener); Line 139: Line 140: if (this.model != null) { Line 141: this.model.getPropertyChangedEvent().removeListener(availabilityListener); Line 142: } Line 143: this.model = model; > why persisting the model instance? just pass it to 'toggeVisibility' same Line 144: model.getPropertyChangedEvent().addListener(availabilityListener); Line 145: toggleVisibility(); Line 146: } Line 147: http://gerrit.ovirt.org/#/c/29530/6/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/datacenter/SubTabDataCenterStorageQosView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/datacenter/SubTabDataCenterStorageQosView.java: Line 35: initWidget(getTable()); Line 36: } Line 37: Line 38: void initTable(final ApplicationConstants constants) { Line 39: getTable().enableColumnResizing(); > add column sorting will do it in a separate patch. Line 40: Line 41: getTable().addColumn(new TextColumnWithTooltip<StorageQos>() { Line 42: @Override Line 43: public String getValue(StorageQos object) { -- 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
