----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54339/#review158420 -----------------------------------------------------------
Fix it, then Ship it! Overall, the patch looks fine to me. Just a few minor issues that need some clarification/possible changes. Thanks. ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/web/listener/LogSearchSessionListener.java (line 31) <https://reviews.apache.org/r/54339/#comment229184> Shouldn't access to this static be synchronized? Since this class is used mainly for debugging the number of existing sessions, this may not be a big deal (so I won't raise an issue, just making a comment), but just wanted to bring it up just in case. Thanks. ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LoggingCookieStore.java (line 31) <https://reviews.apache.org/r/54339/#comment229183> I'm a little concerned that this CookieStore implementation doesn't have any bounds associated with it. Currently, this wouldn't be a problem, but if other classes in this package were to use the same singleton to store cookies with different names, this could eventually lead to a memory problem in the most extreme case. This may not be something to worry about now, but this may be something to consider in the future. ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LoggingRequestHelperImpl.java (line 149) <https://reviews.apache.org/r/54339/#comment229187> The fact that this is a singleton means that we could potentially run into problems with unit tests later on, since multiple test methods for this class could affect the outcome of other tests. The usage of the NetworkConnection interface may mean this isn't a problem, but I think it is at least worth revisiting. Thanks. - Robert Nettleton On Dec. 5, 2016, 2:10 p.m., Oliver Szabo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54339/ > ----------------------------------------------------------- > > (Updated Dec. 5, 2016, 2:10 p.m.) > > > Review request for Ambari, Miklos Gergely and Robert Nettleton. > > > Bugs: AMBARI-19075 > https://issues.apache.org/jira/browse/AMBARI-19075 > > > Repository: ambari > > > Description > ------- > > add cookie management for logsearch integration code. > > also contains a few fixes: > - inactive session timeout is in seconds, not in minutes > - enable info logging in log files (hard to find/debug issues if there is any > problem with logsearch portal) > > > Diffs > ----- > > > ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/LogSearch.java > 2c3f4f5 > > ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/web/listener/LogSearchSessionListener.java > PRE-CREATION > ambari-logsearch/docker/test-config/logsearch/log4j.xml b80824b > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LoggingCookieStore.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LoggingRequestHelperImpl.java > eab0c04 > > ambari-server/src/main/resources/common-services/LOGSEARCH/0.5.0/properties/logsearch-log4j.xml.j2 > ce39030 > > Diff: https://reviews.apache.org/r/54339/diff/ > > > Testing > ------- > > testing done. (FT: manually, UT: one test failing but unrelated with this > change) > > > Thanks, > > Oliver Szabo > >
