> 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. > > Oliver Szabo wrote: > 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)
Thanks for the clarification. As long as we're sure this won't be an issue with memory management in the long term, this change should be fine. I'll drop the issue. - Robert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54339/#review158420 ----------------------------------------------------------- On Dec. 8, 2016, 11:03 a.m., Oliver Szabo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54339/ > ----------------------------------------------------------- > > (Updated Dec. 8, 2016, 11:03 a.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 > > ambari-server/src/test/java/org/apache/ambari/server/controller/logging/LoggingRequestHelperImplTest.java > b839b64 > > 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 > >
