anmolbabu has posted comments on this change. Change subject: engine : volume profile export to pdf command ......................................................................
Patch Set 9: (11 comments) http://gerrit.ovirt.org/#/c/28000/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/CreatePdf.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/CreatePdf.java: Line 45: } Line 46: Line 47: public PdfPTable createTableWithData(String[] columnHeaders, int noOfHeaderRows, String[] data) { Line 48: PdfPTable table = new PdfPTable(columnHeaders.length); Line 49: for (int i = 0; i < columnHeaders.length; i++) { > for (String header : columnHeaders) ?? Done Line 50: PdfPCell tableColumnCell = new PdfPCell(new Phrase(columnHeaders[i])); Line 51: tableColumnCell.setHorizontalAlignment(Element.ALIGN_CENTER); Line 52: table.addCell(tableColumnCell); Line 53: } Line 51: tableColumnCell.setHorizontalAlignment(Element.ALIGN_CENTER); Line 52: table.addCell(tableColumnCell); Line 53: } Line 54: table.setHeaderRows(noOfHeaderRows); Line 55: for (int i = 0; i < data.length; i++) { > for (String item : data) ?? Done Line 56: table.addCell(data[i]); Line 57: } Line 58: return table; Line 59: } Line 58: return table; Line 59: } Line 60: Line 61: public PdfPTable populateDataIntoTable(PdfPTable table, String[] data) { Line 62: for (int i = 0; i < data.length; i++) { > for (String item: data) ?? Done Line 63: table.addCell(data[i]); Line 64: } Line 65: return table; Line 66: } http://gerrit.ovirt.org/#/c/28000/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterVolumeProfilePdfExportCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterVolumeProfilePdfExportCommand.java: Line 30: public class GlusterVolumeProfilePdfExportCommand extends GlusterCommandBase<GlusterVolumeCreatePdfParameters> { Line 31: Line 32: CreatePdf createPdf; Line 33: Line 34: String[] profileColumnHeaders = { "File operation", "No. of Invocations", "Max-Latency", "Min-Latency", "Avg-Latency" }; > What abt Alexender's comments? Done Line 35: Line 36: public GlusterVolumeProfilePdfExportCommand(GlusterVolumeCreatePdfParameters params) { Line 37: super(params); Line 38: } Line 47: try { Line 48: createVolumeProfileDoc(); Line 49: setSucceeded(true); Line 50: } catch (DocumentException | IOException e) { Line 51: e.printStackTrace(); > Why printStackTrace ?? Cant we raise proper engine exception ? Done Line 52: } Line 53: } Line 54: Line 55: private void createVolumeProfileDoc() throws DocumentException, IOException { Line 67: document.close(); Line 68: setSucceeded(true); Line 69: VdcReturnValueBase returnValue = new VdcReturnValueBase(); Line 70: returnValue.setDescription("Pdf Saved"); Line 71: returnValue.setSucceeded(true); > Where is the variable returnValue used ? Nowhere I need to remove this.Initially I was using it to set byte array into it.Now its no more of any use. Line 72: setActionReturnValue(byteStream.toByteArray()); Line 73: } catch (Exception e) { Line 74: setSucceeded(false); Line 75: setActionReturnValue(e.toString()); Line 112: } Line 113: try { Line 114: document.add(brickProfileDetails); Line 115: } catch (DocumentException e) { Line 116: e.printStackTrace(); > ?? Done Line 117: } Line 118: } Line 119: Line 120: private void exportNfsProfileDetails(Document document, List<NfsProfileDetails> nfsProfiles) Line 116: e.printStackTrace(); Line 117: } Line 118: } Line 119: Line 120: private void exportNfsProfileDetails(Document document, List<NfsProfileDetails> nfsProfiles) > How far this method is different from above one? Looks almost similar. Can Both of them are operating on 2 completely different datatype/entities.Or May be I have to think of a base class for BrickProfileDetails and NfsProfileDetails thereby reduce the duplicates. Line 121: throws BadElementException { Line 122: Anchor nfsProfileAnchor = new Anchor("Nfs Profile Details"); Line 123: nfsProfileAnchor.setName("Nfs Profile Details"); Line 124: Chapter nfsProfileDetails = new Chapter(new Paragraph(nfsProfileAnchor), 1); Line 159: } Line 160: try { Line 161: document.add(nfsProfileDetails); Line 162: } catch (DocumentException e) { Line 163: e.printStackTrace(); > ?? Done Line 164: } Line 165: } Line 166: Line 167: private List<String> lineariseStats(StatsInfo statsInfo) { Line 167: private List<String> lineariseStats(StatsInfo statsInfo) { Line 168: return stringifyAndLineariseFopStats(statsInfo.getFopStats()); Line 169: } Line 170: Line 171: private List<String> stringifyAndLineariseFopStats(List<FopStats> fopsStats) { > Think of better name may be. Looks bit funny to me. Done.Any suggestions? Line 172: List<String> fopStatsStringified = new ArrayList<String>(); Line 173: for (FopStats fopStats : fopsStats) { Line 174: fopStatsStringified.add(fopStats.getName()); Line 175: fopStatsStringified.add(fopStats.getHits() + ""); http://gerrit.ovirt.org/#/c/28000/9/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/gluster/GlusterVolumeCreatePdfParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/gluster/GlusterVolumeCreatePdfParameters.java: Line 10: private GlusterVolumeProfileInfo profileInfo; Line 11: private String filePath; Line 12: private String userId; Line 13: Line 14: public static enum ExportData { > Mostly enum are defined all caps with underscores. Done Line 15: NFSProfile, Line 16: BrickProfile; Line 17: }; Line 18: -- To view, visit http://gerrit.ovirt.org/28000 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I208a77456487a5f735948d6cf9fec54f004b6e89 Gerrit-PatchSet: 9 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: anmolbabu <[email protected]> Gerrit-Reviewer: Alexander Wels <[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
