dsmiley commented on a change in pull request #264: URL: https://github.com/apache/solr/pull/264#discussion_r694311373
########## File path: solr/core/src/test/org/apache/solr/handler/admin/ShowFileRequestHandlerTest.java ########## @@ -84,7 +84,7 @@ public void testDirList() throws SolrServerException, IOException { public void testGetRawFile() throws SolrServerException, IOException { SolrClient client = getSolrClient(); //assertQ(req("qt", "/admin/file")); TODO file bug that SolrJettyTestBase extends SolrTestCaseJ4 - QueryRequest request = new QueryRequest(params("file", "managed-schema.xml")); + QueryRequest request = new QueryRequest(params("file", "managed-schema")); Review comment: Changed your mind? ########## File path: solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java ########## @@ -223,15 +257,9 @@ private InputStream readSchemaLocally() { InputStream schemaInputStream = null; try { // Attempt to load the managed schema - try { - schemaInputStream = loader.openResource(managedSchemaResourceName); - } - catch (IOException e) { - // Attempt to load the managed schema with the legacy name. - log.info("The schema is configured as managed, but managed schema resource {} not found - looking for legacy {} instead" - , managedSchemaResourceName, LEGACY_MANAGED_SCHEMA_RESOURCE_NAME); - schemaInputStream = loader.openResource(LEGACY_MANAGED_SCHEMA_RESOURCE_NAME); - } + final String managedSchemaPath = lookupLocalManagedSchemaPath().toString(); + schemaInputStream = loader.openResource(lookupLocalManagedSchemaPath().toString()); + managedSchemaResourceName = managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/")+1); // not loving this Review comment: > // not loving this Me neither! Firstly, please avoid File in Java; use Path henceforth. `lookupLocalManagedSchemaPath` ought to return a *Path* not *File*. Path gives you methods to get the last piece via `getName(getNameCount()-1)` Also, I see no reason to call `lookupLocalManagedSchemaPath` twice as you have done. ########## File path: solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java ########## @@ -91,6 +92,33 @@ public void init(NamedList<?> args) { public String getSchemaResourceName(String cdResourceName) { return managedSchemaResourceName; // actually a guess; reality depends on the actual files in the config set :-( } + + /** + * Lookup the path to the managed schema, dealing with falling back to the + * legacy managed-schema file, instead of the expected managed-schema.xml file. + * + * This method is duplicated in ManagedIndexSchema. + * @see org.apache.solr.schema.ManagedIndexSchema#lookupManagedSchemaPath + */ + public String lookupManagedSchemaPath() { + final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader)loader; Review comment: @sonatype-lift ignore ########## File path: solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java ########## @@ -91,6 +92,33 @@ public void init(NamedList<?> args) { public String getSchemaResourceName(String cdResourceName) { return managedSchemaResourceName; // actually a guess; reality depends on the actual files in the config set :-( } + + /** + * Lookup the path to the managed schema, dealing with falling back to the + * legacy managed-schema file, instead of the expected managed-schema.xml file. + * + * This method is duplicated in ManagedIndexSchema. + * @see org.apache.solr.schema.ManagedIndexSchema#lookupManagedSchemaPath + */ + public String lookupManagedSchemaPath() { + final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader)loader; + final ZkController zkController = zkLoader.getZkController(); + final SolrZkClient zkClient = zkController.getZkClient(); + String managedSchemaPath = zkLoader.getConfigSetZkPath() + "/" + managedSchemaResourceName; + try { + // check if we are using the legacy managed-schema file name. + if (!zkClient.exists(managedSchemaPath, true)){ + log.info("Managed schema resource {} not found - loading legacy managed schema {} file instead" + , managedSchemaResourceName, ManagedIndexSchemaFactory.LEGACY_MANAGED_SCHEMA_RESOURCE_NAME); + managedSchemaPath = zkLoader.getConfigSetZkPath() + "/" + ManagedIndexSchemaFactory.LEGACY_MANAGED_SCHEMA_RESOURCE_NAME; + } + } catch (KeeperException e) { + throw new RuntimeException(e); + } catch (InterruptedException e) { + throw new RuntimeException(e); Review comment: > I don't know how you knew to set that however! This is a standard Java practice that is easy to miss. I wish Sonatype Lift / would raise alarms about this in case I'm not a code reviewer :-). There is one bug pattern used by error-prone (Sonatype Lift calls it) -- https://errorprone.info/bugpattern/InterruptedExceptionSwallowed but you didn't _quite_ run a-fowl of that pattern since you did explicitly catch InterruptedException. Error-prone may be assuming that because you explicitly caught it, that you know what you are doing ;-) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org