Ravi Nori has posted comments on this change. Change subject: engine, webadmin: Webadmin read reports.xml from remote reports app ......................................................................
Patch Set 10: (11 comments) http://gerrit.ovirt.org/#/c/29723/10//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2014-07-08 15:56:35 -0400 Line 4: Commit: Ravi Nori <[email protected]> Line 5: CommitDate: 2014-07-09 12:54:31 -0400 Line 6: Line 7: engine, webadmin: Webadmin needs to be able to read reports.xml via GET from remote reports app > Too long comment line. It doesn't fit the box in the gerrit's gui... Done Line 8: Line 9: in 3.5 report can be installed in separate server. Line 10: Webadmin needs to be able to read reports.xml from Line 11: that location via http. http://gerrit.ovirt.org/#/c/29723/10/backend/manager/modules/services/src/main/java/org/ovirt/engine/core/services/ReportsXmlServlet.java File backend/manager/modules/services/src/main/java/org/ovirt/engine/core/services/ReportsXmlServlet.java: Line 19: * from engine_var/reports.xml as the reports web app can be Line 20: * installed on a remote machine Line 21: */ Line 22: @SuppressWarnings("serial") Line 23: public class ReportsXmlServlet extends HttpServlet { > This class duplicates most of the code of ReportsStatusServlet. Please use Done Line 24: Line 25: @Override Line 26: protected void doGet(HttpServletRequest request, HttpServletResponse response) Line 27: throws ServletException, IOException { Line 46: copy > Is there any reason not using IOUtils.copy(in,out) instead of implementing I tried IOUtils.copy first, it did not work. I had to copy the code from IOUtils copy into this class to get it working. I can't explain why it doesn't work http://gerrit.ovirt.org/#/c/29723/10/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/ReportInit.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/ReportInit.java: Line 57: // the re-init can happen after logout/login. Line 58: // As this class has it's state, it needs to be inited again Line 59: initState(); Line 60: Line 61: AsyncDataProvider.getRedirectServletReportsPage(new AsyncQuery(this, > The callback is not needed any more (the values are cached). AsyncDataProvi Done Line 62: new INewAsyncCallback() { Line 63: @Override Line 64: public void onSuccess(Object target, Object returnValue) { Line 65: setReportBaseUrl((String) returnValue); Line 97: return dashboardMap.get(type); Line 98: } Line 99: Line 100: private void scheduleCheckStatus() { Line 101: if (urlInitialized && reportBaseUrl.trim().isEmpty()) { > IMO, urlInitialized check here is redundant. Moved it below to make it clear Line 102: return; Line 103: } Line 104: if (scheduledStatusCheckInProgress) { Line 105: return; Line 100: private void scheduleCheckStatus() { Line 101: if (urlInitialized && reportBaseUrl.trim().isEmpty()) { Line 102: return; Line 103: } Line 104: if (scheduledStatusCheckInProgress) { > Consider adding (retryCount > MAX_RETRY_COUNTS || reportsWebappDeployed) to Done Line 105: return; Line 106: } Line 107: scheduledStatusCheckInProgress = true; Line 108: Scheduler.get().scheduleFixedDelay( Line 108: Scheduler.get().scheduleFixedDelay( Line 109: new Scheduler.RepeatingCommand() { Line 110: @Override Line 111: public boolean execute() { Line 112: if (retryCount > MAX_RETRY_COUNTS || reportsWebappDeployed) { > if (retryCount > MAX_RETRY_COUNTS) then setXmlInitialized() should be calle Not required to call setXmlInitialized here, it is called in parseReportsXML onError after call to scheduleCheckStatus() Line 113: scheduledStatusCheckInProgress = false; Line 114: return false; Line 115: } Line 116: retryCount++; Line 147: } Line 148: } Line 149: Line 150: private void parseReportsXML() { Line 151: RequestBuilder requestBuilder = > There is just one small difference between this requestBuilder and the stat I dont see a point of creating a new method for creating a RequestBuilder but I have added a new method public static RequestBuilder constructRequestBuilder(String url) { return new RequestBuilder(RequestBuilder.GET, url); } Line 152: new RequestBuilder(RequestBuilder.GET, Line 153: "/" //$NON-NLS-1$ Line 154: + BaseContextPathData.getInstance().getRelativePath() Line 155: + "services/reports-xml"); //$NON-NLS-1$ Line 157: requestBuilder.sendRequest(null, new RequestCallback() { Line 158: @Override Line 159: public void onError(Request request, Throwable exception) { Line 160: scheduleCheckStatus(); Line 161: setXmlInitialized(); > Calling 'setXmlInitialized()' can raise 'reportsInitEvent', on which the lo Added a new boolean to raise event just once. To answer your second question Let me explain the flow, which is modified from your suggestions 1. init sets url by getting value from cache and calls parseReportsXML. So urlInitialized is not required 2. parseReportsXml sends a request to get the reports.xml. onFailure scheduleCheckStatus is called if reportBaseUrl is not empty. setXmlInitialized is called 3. onSuccess if status is OK the xml is parsed. if status is NOT_FOUND scheduleCheckStatus is called if reportBaseUrl is not empty. In the finally block setXmlInitialized is called. scheduleCheckStatus checks if scheduledStatusCheckInProgress or reportsWebappDeployed and schedules checking status at a fixed rate. The second check to reportsWebappDeployed (as suggested by you) will stop indefinite execution of scheduleCheckStatus from parseXml onError. A new flag initEventRaised makes sure that reportsInitEvent is raised only once after the reports becomes available. The login no longer waits until reports.xml is retrieved from the reports web app. When the reportsInitEvent is raised the CommonModel's updateAvailability method is called and the reports tab and reports actions become available Line 162: } Line 163: Line 164: @Override Line 165: public void onResponseReceived(Request request, Response response) { Line 181: setXmlInitialized > Same here. Added a new flag to raise event just once http://gerrit.ovirt.org/#/c/29723/10/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/tab/MainTabReportsPresenter.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/tab/MainTabReportsPresenter.java: Line 69: } Line 70: Line 71: private void checkUpdateReportsPanel(final String url, Line 72: final Map<String, List<String>> params) { Line 73: RequestBuilder requestBuilder = > You can use here the method I suggested to add in reportInit. Done Line 74: new RequestBuilder(RequestBuilder.GET, buildStatusUrl()); Line 75: try { Line 76: requestBuilder.sendRequest(null, new RequestCallback() { Line 77: @Override -- To view, visit http://gerrit.ovirt.org/29723 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I76db7ab889f21de083bb3c8276e8abb77b68fdb3 Gerrit-PatchSet: 10 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Ravi Nori <[email protected]> Gerrit-Reviewer: Shirly Radco <[email protected]> Gerrit-Reviewer: Yaniv Dary <[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
