> On Dec. 7, 2016, 10:25 p.m., Robert Nettleton wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LoggingRequestHelperImpl.java, > > line 151 > > <https://reviews.apache.org/r/54339/diff/3/?file=1576401#file1576401line151> > > > > 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.
In tests, we can set the map data to empty in that case > On Dec. 7, 2016, 10:25 p.m., Robert Nettleton wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LoggingCookieStore.java, > > line 31 > > <https://reviews.apache.org/r/54339/diff/3/?file=1576400#file1576400line31> > > > > 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. from the logsearch side we have 2 cookies (JSESSIONID and msa ... note that we should rename JSESSIONID, i will do that in a different patch). also we are not storing cookies per url. so mostly these 2 elements are the same for every call. I used names as keys and we need to use cookie storage only for LogSearch calls...therefore it will have maximum 2 elements (also: every time we update the elements). only way to hit a memory issue if every time we got a lots of new cookies in every call. it does not sound like a real scenario. (if we would use the java.net CookieManager it would be worse anyway). I think overall its better to use a map for that because we wont depends on the sessionid name in the integration code. (we could use just a string instead of a map to have bounds as you said) > On Dec. 7, 2016, 10:25 p.m., Robert Nettleton wrote: > > ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/web/listener/LogSearchSessionListener.java, > > line 31 > > <https://reviews.apache.org/r/54339/diff/3/?file=1576398#file1576398line31> > > > > 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. thanks, i will fix that. - Oliver ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54339/#review158420 ----------------------------------------------------------- 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 > >
