----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69330/#review210704 -----------------------------------------------------------
client/src/main/java/org/apache/oozie/client/AuthOozieClient.java Lines 73 (patched) <https://reviews.apache.org/r/69330/#comment295439> Can be `final`. client/src/main/java/org/apache/oozie/client/AuthOozieClient.java Lines 100 (patched) <https://reviews.apache.org/r/69330/#comment295440> Use `@VisibleForTesting`, and can be package-private. client/src/main/java/org/apache/oozie/client/AuthOozieClient.java Lines 101 (patched) <https://reviews.apache.org/r/69330/#comment295441> Why not use Guava's `BaseEncoding` like this? ``` BaseEncoding .base64Url() .omitPadding() .encode("https://localhost:11443/oozie".getBytes(Charsets.UTF_8)) ``` core/src/main/java/org/apache/oozie/test/EmbeddedServletContainer.java Lines 131-133 (patched) <https://reviews.apache.org/r/69330/#comment295442> Can you please extract to a well-named method, or leave a code comment for future maintainers, or both? core/src/main/java/org/apache/oozie/test/EmbeddedServletContainer.java Line 128 (original), 136 (patched) <https://reviews.apache.org/r/69330/#comment295443> Can you please extract to a well-named method, or leave a code comment for future maintainers, or both? core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java Lines 312-336 (patched) <https://reviews.apache.org/r/69330/#comment295444> Can you please: * extract to separate test method * provide assertion messages - András Piros On Nov. 14, 2018, 2:02 a.m., zhang junfan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69330/ > ----------------------------------------------------------- > > (Updated Nov. 14, 2018, 2:02 a.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/1/ > > > 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. > > > Thanks, > > zhang junfan > >
