----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70057/#review213222 -----------------------------------------------------------
embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/SolrSetupUtil.java Lines 97 (patched) <https://reviews.apache.org/r/70057/#comment299102> The convention for constants in Java is full upper case, eg CONFIG_FILES embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/SolrSetupUtil.java Lines 101 (patched) <https://reviews.apache.org/r/70057/#comment299103> The convention for constants in Java is full upper case, eg CONFIG_FILE embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/SolrSetupUtil.java Lines 111 (patched) <https://reviews.apache.org/r/70057/#comment299105> This can be a constructor - without the startProcess call, and maybe taking a 'configFile' parameter. This way, most of the variables can be set to final. embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/SolrSetupUtil.java Lines 169 (patched) <https://reviews.apache.org/r/70057/#comment299104> Tabs and whitespaces are a bit inconsistently used. embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/SolrSetupUtil.java Lines 184 (patched) <https://reviews.apache.org/r/70057/#comment299108> I would rather re-throw an exception from here, and handle it with System.exit in the EmbeddedWebServer code. Or just remove this catch, and allow throwing the exception to the caller as is. embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/SolrSetupUtil.java Lines 203 (patched) <https://reviews.apache.org/r/70057/#comment299117> I guess, the log messages are copy pasted from getCollections() call :-) embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/SolrSetupUtil.java Lines 204 (patched) <https://reviews.apache.org/r/70057/#comment299106> I don't get, why do you want to return an empty list here - I guess, you can't do anything after this, the code will only report, that 'Core with name ... Already exists' - which is misleading. I would rather not catch these exceptions here. embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/SolrSetupUtil.java Lines 240 (patched) <https://reviews.apache.org/r/70057/#comment299107> Catching Throwable is not too good solution - if you really want to log problems here, please pick a more specific Exception, and rethrow it after the log. Or just remove this catch, and throw the exception to the caller as is. embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/SolrSetupUtil.java Lines 258 (patched) <https://reviews.apache.org/r/70057/#comment299109> solrClient is never null here - it is just initialized to non-null a line before. embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/SolrSetupUtil.java Lines 278 (patched) <https://reviews.apache.org/r/70057/#comment299110> Catching Throwable is not too good solution - if you really want to log problems here, please pick a more specific Exception, and rethrow it after the log. Or just remove this catch, and throw the exception to the caller as is - I mean, the original, specific exception embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/SolrSetupUtil.java Lines 291 (patched) <https://reviews.apache.org/r/70057/#comment299111> You can use java.nio.file.FileSystem more easily: File tmpDir = FileSystem.getPath(System.getProperty("java.io.tmpdir"), UUID.randomUUID().toString(), "RangerAudit")).toFile(); embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/SolrSetupUtil.java Lines 308 (patched) <https://reviews.apache.org/r/70057/#comment299118> Catching Throwable is not too good solution - if you really want to log problems here, please pick a more specific Exception, and rethrow it after the log. Or just remove this catch, and throw the exception to the caller as is - I mean, the original, specific exception embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/SolrSetupUtil.java Lines 334 (patched) <https://reviews.apache.org/r/70057/#comment299112> Am I right, that you just want to upload one file, which name match getConfigFileName()? If yes, then it's simpler to check if 'getConfigSetFolder()/getConfigFileName()' exists, and just upload that. embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/SolrSetupUtil.java Lines 356 (patched) <https://reviews.apache.org/r/70057/#comment299113> You can use Path/FileSystem directly as in previous locations embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/SolrSetupUtil.java Lines 368 (patched) <https://reviews.apache.org/r/70057/#comment299114> You can use Path/FileSystem directly as in previous locations embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/SolrSetupUtil.java Lines 374 (patched) <https://reviews.apache.org/r/70057/#comment299115> You can use Path/FileSystem directly as in previous locations embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/SolrSetupUtil.java Lines 415 (patched) <https://reviews.apache.org/r/70057/#comment299119> Catching Throwable is not too good solution - if you really want to log problems here, please pick a more specific Exception, and rethrow it after the log. Or just remove this catch, and throw the exception to the caller as is - I mean, the original, specific exception embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/SolrSetupUtil.java Lines 510 (patched) <https://reviews.apache.org/r/70057/#comment299116> Catching Exception is not too good solution - if you really want to log problems here, please pick a more specific exception, and rethrow it after the log. Or just remove this catch, and throw the exception to the caller as is - I mean, the original, specific exception embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/SolrSetupUtil.java Lines 531 (patched) <https://reviews.apache.org/r/70057/#comment299120> I don't think, that you can do anything, if you can't talk to Solr, and you don't get the collections, I would rather have the exception propagated to the top. - Zsombor Gegesy On Feb. 26, 2019, 10:17 a.m., bhavik patel wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70057/ > ----------------------------------------------------------- > > (Updated Feb. 26, 2019, 10:17 a.m.) > > > Review request for ranger, Ankita Sinha, Don Bosco Durai, Gautam Borad, Abhay > Kulkarni, Madhan Neethiraj, Oliver Szabo, Pradeep Agrawal, Ramesh Mani, > Selvamohan Neethiraj, Sailaja Polavarapu, and Velmurugan Periasamy. > > > Bugs: RANGER-2324 > https://issues.apache.org/jira/browse/RANGER-2324 > > > Repository: ranger > > > Description > ------- > > We are handling the solr bootstrapping in below mentioned manner for Ranger > 1.) Connection to solr > 2.) Upload Configuration > 3.) Create Collection > 4.) Setting ACL > > > Diffs > ----- > > > embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/EmbeddedServer.java > 8d32352 > > embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/SolrSetupUtil.java > PRE-CREATION > security-admin/scripts/install.properties fdcee1b > security-admin/scripts/setup.sh bd4bd4c > security-admin/src/main/resources/conf.dist/ranger-admin-site.xml 4d4a1de > src/main/assembly/admin-web.xml 8ea728b > > > Diff: https://reviews.apache.org/r/70057/diff/2/ > > > Testing > ------- > > Tested Below Scenario on ranger manual start / restart > 1.) Solr configuration were uploaded successfully > 2.) Solr collections were created successfully > 3.) ACL were setup as required. > > > Thanks, > > bhavik patel > >
