----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51242/#review146193 -----------------------------------------------------------
Fix it, then Ship it! Overall, the change looks like a good addition/change to the LogSearch REST API. I have a few issues I've raised below, regarding the build-level dependencies and Apache licensing concerns. Thanks. ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/rest/ServiceLogsREST.java (line 29) <https://reviews.apache.org/r/51242/#comment212575> Maybe I've missed something, but I can't find the build-level changes to introduce Swagger into the Ambari build. Is this change introduced in a separate patch? ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/rest/ServiceLogsREST.java (line 30) <https://reviews.apache.org/r/51242/#comment212576> Another thing to consider: Are we sure that all of the Swagger components required by this change in Ambari are using the correct Apache licensing? Some initial searches I've done online seem to indicate that this code is Apache-licensed, but we'll probably need to verify that for all usages/inclusions of Swagger in Ambari. - Robert Nettleton On Aug. 19, 2016, 3:26 p.m., Oliver Szabo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51242/ > ----------------------------------------------------------- > > (Updated Aug. 19, 2016, 3:26 p.m.) > > > Review request for Ambari, Dharmesh Makwana, Miklos Gergely, Robert > Nettleton, and Sumit Mohanty. > > > Bugs: AMBARI-18214 > https://issues.apache.org/jira/browse/AMBARI-18214 > > > Repository: ambari > > > Description > ------- > > - restify endpoints as possible > - rename dashboard to service > - there are still some endpoints what is not too resty, but that is the "vol > 1" for refactoring. in some cases, some of the endpoints can be merged into > one, and call a same method with just different params. for now, i kept them, > i did not want to do any changes in the business code part. (after Miklos is > ready with the refactoring, we can do some changes there too, and get rid > some weird names in the API ... like verbs e.g.: /service/logs/aggregated -> > it can be also in /service/logs endpoint and add aggregated as a query param > etc.) > > > Diffs > ----- > > > ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/dao/SolrDaoBase.java > 672507a > > ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/doc/DocConstants.java > c1572b7 > > ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/manager/AuditMgr.java > d4f2986 > > ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/manager/LogsMgr.java > b03a643 > > ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/rest/AuditLogsREST.java > PRE-CREATION > > ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/rest/AuditREST.java > 5ed49fd > > ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/rest/DashboardREST.java > 0144edc > > ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/rest/LogFileREST.java > d53cff9 > > ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/rest/PublicREST.java > af48acd > > ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/rest/ServiceLogsREST.java > PRE-CREATION > > ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/rest/UserConfigREST.java > 4b1675f > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/login.html 44f1aeb > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/collection_bases/VAuditLogListBase.js > 12f7c31 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/collection_bases/VEventHistoryListBase.js > f6f720d > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/collection_bases/VGroupListBase.js > a34aaa3 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/collection_bases/VLogLevelListBase.js > 59b5ae8 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/collection_bases/VLogListBase.js > 7b102d5 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/collection_bases/VNameValueListBase.js > d59eaa2 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/collection_bases/VNodeListBase.js > 7c7dcf8 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/model_bases/VAuditLogBase.js > 1283875 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/model_bases/VCommonModelBase.js > 4723e3e > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/model_bases/VEventHistoryBase.js > c237ade > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/model_bases/VGraphInfoBase.js > a707629 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/model_bases/VLogLevelBase.js > 0384bc2 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/model_bases/VUserFilterBase.js > 35171aa > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/utils/ViewUtils.js > 331ffd6 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/views/audit/AuditAggregatedView.js > c04aaf9 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/views/audit/AuditTabLayoutView.js > 0b570ac > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/views/dashboard/BubbleGraphTableLayoutView.js > 42b94d5 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/views/dashboard/ComponentListView.js > abd3740 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/views/dashboard/ComponentsView.js > 66cc277 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/views/dashboard/DashboardView.js > c3fd9c2 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/views/dashboard/GridTableLayoutView.js > 1cbdef8 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/views/dashboard/HostsView.js > dd82130 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/views/dashboard/LogLevelBoxView.js > b57f7c1 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/views/dashboard/MainLayoutView.js > 3be87f5 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/views/dialog/GlobalExclusionCompositeView.js > a737eba > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/views/filter/CreateLogfeederFilterView.js > ba07600 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/views/graphs/GraphLayoutView.js > 0085f06 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/views/graphs/GridGraphLayoutView.js > b0339f3 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/views/tabs/ComparisonView.js > 1d26dc4 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/views/tabs/HierarchyTabLayoutView.js > 6c6a77e > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/views/tabs/LogFileView.js > 4af4670 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/views/tabs/TreeView.js > cf33e68 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/scripts/views/troubleshoot/TroubleShootLayoutView.js > c1655f0 > > ambari-logsearch/ambari-logsearch-portal/src/main/webapp/templates/graphs/backup.js > 9b589f1 > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LoggingRequestHelperImpl.java > 276a65e > > Diff: https://reviews.apache.org/r/51242/diff/ > > > Testing > ------- > > manual testing with the UI > > > Thanks, > > Oliver Szabo > >
