anmolbabu has posted comments on this change. Change subject: engine : volume profile export to pdf command ......................................................................
Patch Set 17: (11 comments) http://gerrit.ovirt.org/#/c/28000/17/backend/manager/dependencies/pom.xml File backend/manager/dependencies/pom.xml: Line 233: Line 234: <dependency> Line 235: <groupId>com.itextpdf</groupId> Line 236: <artifactId>itextpdf</artifactId> Line 237: <version>5.5.1</version> > you should define this version in properties section of pom.xml at project Done Line 238: </dependency> Line 239: Line 240: </dependencies> Line 241: http://gerrit.ovirt.org/#/c/28000/17/backend/manager/modules/bll/pom.xml File backend/manager/modules/bll/pom.xml: Line 161: </dependency> Line 162: <dependency> Line 163: <groupId>com.itextpdf</groupId> Line 164: <artifactId>itextpdf</artifactId> Line 165: <version>5.5.1</version> > same comment here as backend/../pom.xml Done Line 166: <scope>provided</scope> Line 167: </dependency> Line 168: </dependencies> Line 169: Line 162: <dependency> Line 163: <groupId>com.itextpdf</groupId> Line 164: <artifactId>itextpdf</artifactId> Line 165: <version>5.5.1</version> Line 166: <scope>provided</scope> > Is this scope required? Ya I think this provides the library at run time.i.e, there was a place in Backend.java where we iterate over all the packages and instantiate CreatePdf class(Class.forName(...)).So, I made the scope provided just to make the class available at run time.I'll remove this and check once.The default scope is compile time I guess. Line 167: </dependency> Line 168: </dependencies> Line 169: Line 170: <build> http://gerrit.ovirt.org/#/c/28000/17/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 13: import com.itextpdf.text.Phrase; Line 14: import com.itextpdf.text.pdf.PdfPCell; Line 15: import com.itextpdf.text.pdf.PdfPTable; Line 16: Line 17: public class CreatePdf { > Too generic. Also can you rename the class so that it's not like a method? Done.I will rename it as PdfCreator and move it to utils as you said. Line 18: public static final Font headingFont = new Font(Font.FontFamily.TIMES_ROMAN, 18, Font.BOLD); Line 19: public static final Font subHeadingFont = new Font(Font.FontFamily.TIMES_ROMAN, 16, Font.BOLD); Line 20: public static final Font paragraphFont = new Font(Font.FontFamily.TIMES_ROMAN, 12, Font.NORMAL); Line 21: Line 49: for(String columnHeader : columnHeaders) { Line 50: PdfPCell tableColumnCell = new PdfPCell(new Phrase(columnHeader)); Line 51: tableColumnCell.setHorizontalAlignment(Element.ALIGN_CENTER); Line 52: table.addCell(tableColumnCell); Line 53: } > code is repeated. please refactor Done Line 54: table.setHeaderRows(noOfHeaderRows); Line 55: for (String item : data) { Line 56: table.addCell(item); Line 57: } Line 53: } Line 54: table.setHeaderRows(noOfHeaderRows); Line 55: for (String item : data) { Line 56: table.addCell(item); Line 57: } > same for above code..exactly same as populateDataIntoTable Done Line 58: return table; Line 59: } Line 60: Line 61: public PdfPTable populateDataIntoTable(PdfPTable table, String[] data) { Line 64: } Line 65: return table; Line 66: } Line 67: Line 68: public List createList(String[] listPoints) { > does this need to be public? I thought of making it public bcoz, I had thought of making a itextpdf library wrapper, that can be used to export any reports in future.Like this class doesn't dictate any specific layout and provides methods to create the layout elements of choice although, it currently provides only a small number of layout elements like list,table. Line 69: List textList = new List(false, false, 10); Line 70: for (String listPoint : listPoints) { Line 71: textList.add(new ListItem(listPoint)); Line 72: } http://gerrit.ovirt.org/#/c/28000/17/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 88: Line 89: List<String> tableContents = lineariseStats(profile.getProfileStats().get(0)); Line 90: profileDetails.add(createPdf.createTableWithData(profileColumnHeaders, 1, tableContents.toArray(new String[tableContents.size()]))); Line 91: Line 92: Pair<Integer, String> runTimeConverted = profile.getProfileStats().get(1).getDurationFormatted(); > possible NPEs and IndexOutofBoundExceptions Done Line 93: Pair<Integer, String> intervalRunTimeConverted = profile.getProfileStats().get(0).getDurationFormatted(); Line 94: Pair<SizeUnit, Double> dataReadInCurrentInterval = SizeConverter.autoConvert(profile.getProfileStats().get(1).getTotalRead(), SizeUnit.BYTES); Line 95: Pair<SizeUnit, Double> dataRead = SizeConverter.autoConvert(profile.getProfileStats().get(0).getTotalRead(), SizeUnit.BYTES); Line 96: Pair<SizeUnit, Double> dataWrittenInCurrentInterval = SizeConverter.autoConvert(profile.getProfileStats().get(1).getTotalWrite(), SizeUnit.BYTES); Line 115: Line 116: } Line 117: Line 118: private List<String> lineariseStats(StatsInfo statsInfo) { Line 119: return getLinearStrings(statsInfo.getFopStats()); > what about the blockStats? Currently unhandled.Will handle it now. Line 120: } Line 121: Line 122: private List<String> getLinearStrings(List<FopStats> fopsStats) { Line 123: List<String> fopStatsStringified = new ArrayList<String>(); Line 128: + fopStats.getMaxLatencyFormatted().getSecond()); Line 129: fopStatsStringified.add(fopStats.getMinLatencyFormatted().getFirst() Line 130: + fopStats.getMinLatencyFormatted().getSecond()); Line 131: fopStatsStringified.add(fopStats.getAvgLatencyFormatted().getFirst() Line 132: + fopStats.getAvgLatencyFormatted().getSecond()); > Don't you need to separate each fopStats? itextpdf library takes the table contents as just a list/array of table cells.And these cells are added as columns to a row until the specified maximum number of columns is reached after this,it creates a new row and repeats the above procedure.So the itextpdf doesn't really understand the contents of each row is in our case essentially different fop objects.It just blindly puts the data containing table cells adjacent to each other until the column maximum is reached.and the column maximum depends on the number of column header Strings and the no. of HeaderRows for which I have a overriden function in createpdf that sets it to 1 bcoz, in most cases no. of header rows for usual table is 1. Line 133: } Line 134: return fopStatsStringified; Line 135: } Line 136: http://gerrit.ovirt.org/#/c/28000/17/backend/manager/modules/common/pom.xml File backend/manager/modules/common/pom.xml: Line 27: Line 28: <dependency> Line 29: <groupId>com.itextpdf</groupId> Line 30: <artifactId>itextpdf</artifactId> Line 31: <version>5.5.1</version> > use external version property Done Line 32: <scope>provided</scope> Line 33: </dependency> Line 34: <dependency> Line 35: <groupId>org.jboss.spec.javax.ejb</groupId> -- 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: 17 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: anmolbabu <anb...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: anmolbabu <anb...@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
