----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69330/#review210926 -----------------------------------------------------------
client/src/main/java/org/apache/oozie/client/AuthOozieClient.java Lines 105 (patched) <https://reviews.apache.org/r/69330/#comment295762> Are you sure you only get characters that fit into a file name on Linux OSes? I think special behavior is needed to mask such incorrect characters. core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java Lines 199-201 (original), 243 (patched) <https://reviews.apache.org/r/69330/#comment295761> Deleting and assertion for non-existence missing. Is it by chance? core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java Lines 320 (patched) <https://reviews.apache.org/r/69330/#comment295758> A couple of other test scenarios would be also of importance: * multiple `OozieCLI` calls to the same Oozie host w/ same context roots * multiple `OozieCLI` calls to the same Oozie host w/ different context roots * multiple `OozieCLI` calls to different Oozie host w/ and different context roots core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java Lines 322-338 (patched) <https://reviews.apache.org/r/69330/#comment295751> Please extract to a well-named method for clarity. core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java Lines 328 (patched) <https://reviews.apache.org/r/69330/#comment295752> What about `try-with-resources` and omitting the `close()` call? core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java Lines 339 (patched) <https://reviews.apache.org/r/69330/#comment295753> Do we have to set this also when `oozie.authentication.signature.secret.file` has been set in beforehand? core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java Lines 343-350 (patched) <https://reviews.apache.org/r/69330/#comment295756> Extract method. core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java Lines 347 (patched) <https://reviews.apache.org/r/69330/#comment295754> Please put appropriate assertion message. core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java Lines 351-358 (patched) <https://reviews.apache.org/r/69330/#comment295757> Extract method. core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java Lines 355 (patched) <https://reviews.apache.org/r/69330/#comment295755> Please put appropriate assertion message. core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java Lines 366 (patched) <https://reviews.apache.org/r/69330/#comment295759> Assertion message. core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java Lines 368 (patched) <https://reviews.apache.org/r/69330/#comment295760> Assertion message. - András Piros On Nov. 28, 2018, 12:15 p.m., zhang junfan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69330/ > ----------------------------------------------------------- > > (Updated Nov. 28, 2018, 12:15 p.m.) > > > Review request for oozie, András Piros and Peter Bacsko. > > > Repository: oozie-git > > > Description > ------- > > Auth token cache file name should include Oozie URL, link to oozie-3379. > > > Diffs > ----- > > client/src/main/java/org/apache/oozie/client/AuthOozieClient.java 3a8b5ab6 > core/src/main/java/org/apache/oozie/test/EmbeddedServletContainer.java > fc9d840b > > core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java > 04fde730 > > > Diff: https://reviews.apache.org/r/69330/diff/3/ > > > Testing > ------- > > I added a test for authOozieClients with the different oozieUrl. > I changed the code for EmbeddedServletContainer. Because this patch needs to > bind the cache file to oozieUrl, you need to test whether multiple different > clients can use the cache. In the original EmbeddedServletContainer code, the > port was random and could not get a given container context path. So the port > binding needs to be added. > > > File Attachments > ---------------- > > oozie-3379-6.patch > > https://reviews.apache.org/media/uploaded/files/2018/11/27/ec465408-bcbc-44bc-a816-7ff73e3c0e68__oozie-3379-6.patch > > > Thanks, > > zhang junfan > >
