Kanagaraj M has posted comments on this change. Change subject: webadmin : Gluster Volume Profile ......................................................................
Patch Set 34: (8 comments) http://gerrit.ovirt.org/#/c/27470/34/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/VolumeProfileStatisticsModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/VolumeProfileStatisticsModel.java: Line 46: Line 47: private VolumeListModel volumeListModel; Line 48: private GlusterVolumeProfileInfo profileInfo; Line 49: Line 50: private static ProfileTabSelected currentTab; Avoid using static fields unless its really required Line 51: Line 52: public static enum ProfileTabSelected { Line 53: BRICKS(0) , Line 54: NFS(1); Line 48: private GlusterVolumeProfileInfo profileInfo; Line 49: Line 50: private static ProfileTabSelected currentTab; Line 51: Line 52: public static enum ProfileTabSelected { Do we need this? Anyway you have one refresh icon per tab. Can you distinguish what to refresh in the event handler for refresh icon? Line 53: BRICKS(0) , Line 54: NFS(1); Line 55: Line 56: private int weight; Line 173: stopProgress(); Line 174: VdcQueryReturnValue vdcValue = (VdcQueryReturnValue) returnValue; Line 175: GlusterVolumeProfileInfo profileInfoEntity =vdcValue.getReturnValue(); Line 176: if((profileInfoEntity == null) || (!vdcValue.getSucceeded())) { Line 177: volumeListModel.showProfileErrorWindow(); is this required? Line 178: onPropertyChanged(new PropertyChangedEventArgs("errorFetchingProfileStats"));//$NON-NLS-1$ Line 179: if(currentTab == ProfileTabSelected.NFS) { Line 180: showNfsProfileStats(profileInfoEntity); Line 181: } else { http://gerrit.ovirt.org/#/c/27470/34/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/volumes/VolumeListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/volumes/VolumeListModel.java: Line 772: private void showVolumeProfiling() { Line 773: if(getSelectedItem() == null) { Line 774: return; Line 775: } Line 776: VolumeProfileStatisticsModel profileStatsModel = new VolumeProfileStatisticsModel(((GlusterVolumeEntity)getSelectedItem()).getClusterId(), ((GlusterVolumeEntity)getSelectedItem()).getId(), ((GlusterVolumeEntity)getSelectedItem()).getName(), this); (GlusterVolumeEntity)getSelectedItem() can be assigned to a local fields, so that avoid casting it again and again Line 777: Line 778: VolumeProfileStatisticsModel.setCurrentTab(VolumeProfileStatisticsModel.ProfileTabSelected.BRICKS); Line 779: setProfileWindow(profileStatsModel); Line 780: profileStatsModel.startProgress(ConstantsManager.getInstance().getConstants().fetchingDataMessage());//$NON-NLS-1$ Line 786: Line 787: profileStatsModel.queryBackend(); Line 788: } Line 789: Line 790: public void showProfileErrorWindow() { is this being used? Line 791: if (getWindow() != null) { Line 792: getWindow().stopProgress(); Line 793: } Line 794: } http://gerrit.ovirt.org/#/c/27470/34/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/VolumeProfileStatisticsPopupView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/VolumeProfileStatisticsPopupView.java: Line 118: Line 119: @UiField Line 120: @Ignore Line 121: @WithElementId Line 122: Label nfsBytesWritten; Can these fields ordered in the way they are displaying in the ui? It makes easier to map :) Line 123: Line 124: private final Driver driver = GWT.create(Driver.class); Line 125: Line 126: @Inject Line 253: ClickHandler brickTabClickHandler = new ClickHandler() { Line 254: @Override Line 255: public void onClick(ClickEvent event) { Line 256: VolumeProfileStatisticsModel.setCurrentTab(VolumeProfileStatisticsModel.ProfileTabSelected.BRICKS); Line 257: object.queryBackend(); you can pass an argument to decide what data to fetch (just boolean would do) instead of using the above static approach Line 258: } Line 259: }; Line 260: Line 261: brickRefreshIcon.setRefreshIconClickListener(brickTabClickHandler); Line 263: ClickHandler nfsTabClickHandler = new ClickHandler() { Line 264: @Override Line 265: public void onClick(ClickEvent event) { Line 266: VolumeProfileStatisticsModel.setCurrentTab(VolumeProfileStatisticsModel.ProfileTabSelected.NFS); Line 267: object.queryBackend(); Same here. you could say false to the bool Line 268: } Line 269: }; Line 270: Line 271: nfsRefreshIcon.setRefreshIconClickListener(nfsTabClickHandler); -- To view, visit http://gerrit.ovirt.org/27470 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic305a0fece18f29d24a9d0324391e484681fa033 Gerrit-PatchSet: 34 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: anmolbabu <[email protected]> Gerrit-Reviewer: Kanagaraj M <[email protected]> Gerrit-Reviewer: Ramesh N <[email protected]> Gerrit-Reviewer: Sahina Bose <[email protected]> Gerrit-Reviewer: Shubhendu Tripathi <[email protected]> Gerrit-Reviewer: anmolbabu <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
