[ https://issues.apache.org/jira/browse/SOLR-11551?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16410678#comment-16410678 ]
Steve Rowe edited comment on SOLR-11551 at 3/23/18 1:48 AM: ------------------------------------------------------------ Thanks for making the try-catch change, looks good. +1 on the rest of the new patch, except for three small things I noticed after I took a closer look at the patch: 1. in {{MergeIndexesOp}}, you changed the line {{if (core != null) \{}} to {{if (core == null) return;}} (line 65). This could happen earlier, and short-circuit some unnecessary collection creations, right after where the core is obtained, on line 55. {code:java} 51: @Override 52: public void execute(CoreAdminHandler.CallInfo it) throws Exception { 53: SolrParams params = it.req.getParams(); 54: String cname = params.required().get(CoreAdminParams.CORE); 55: SolrCore core = it.handler.coreContainer.getCore(cname); 56: SolrQueryRequest wrappedReq = null; 57: 58: List<SolrCore> sourceCores = Lists.newArrayList(); 59: List<RefCounted<SolrIndexSearcher>> searchers = Lists.newArrayList(); 60: // stores readers created from indexDir param values 61: List<DirectoryReader> readersToBeClosed = Lists.newArrayList(); 62: Map<Directory, Boolean> dirsToBeReleased = new HashMap<>(); 63: 64: 65: if (core == null) return; {code} 2. {{CoreAdminOperation.execute()}} no longer needs to declare {{throws Exception}} since it always wraps any encountered exceptions with {{SolrException}}, which is unchecked. As a result, all implementing classes should also remove this declaration. (Note that this detail was included in this issue's description.) 3. Copy/paste-o's in {{CoreAdminOperationTest.java}}: 3.a. Should call {{REJOINLEADERELECTION_OP.execute()}} instead of {{REQUESTSTATUS_OP.execute()}} (and the failure message should be adjusted too): {code:java} public void testRejoinLeaderElectionUnexpectedFailuresResultIn500Exception() { final Throwable cause = new NullPointerException(); whenUnexpectedErrorOccursDuringCoreAdminOp(cause); try { CoreAdminOperation.REQUESTSTATUS_OP.execute(callInfo); fail("Expected request-status execution to fail with exception."); {code} 3.b. failure message prints "core-unload" instead of "core-reload": {code} public void testReloadUnexpectedFailuresResultIn500SolrException() { final Throwable cause = new NullPointerException(); whenUnexpectedErrorOccursDuringCoreAdminOp(cause); try { CoreAdminOperation.RELOAD_OP.execute(callInfo); fail("Expected core-unload execution to fail with exception."); {code} 3.c. failure message prints "request-sync" instead of "request-buffer-updates": {code} public void testRequestBufferUpdatesUnexpectedFailuresResultIn500Exception() { final Throwable cause = new NullPointerException(); whenUnexpectedErrorOccursDuringCoreAdminOp(cause); try { CoreAdminOperation.REQUESTBUFFERUPDATES_OP.execute(callInfo); fail("Expected request-sync execution to fail with exception."); {code} 3.d. failure message prints "request-apply-updates" instead of "request-status": {code} public void testRequestStatusMissingRequestIdParamResultsIn400SolrException() { whenCoreAdminOpHasParams(Maps.newHashMap()); try { CoreAdminOperation.REQUESTSTATUS_OP.execute(callInfo); fail("Expected request-apply-updates execution to fail when no 'requestid' param present"); {code} was (Author: steve_rowe): Thanks for making the try-catch change, looks good. +1 on the rest of the new patch, except for three small things I noticed after I took a closer look at the patch: 1. in {{MergeIndexesOp}}, you changed the line {{if (core != null) \{}} to {{if (core == null) return;}} (line 65). This could happen earlier, and short-circuit some unnecessary collection creations, right after where the core is obtained, on line 55. {code:java} 51: @Override 52: public void execute(CoreAdminHandler.CallInfo it) throws Exception { 53: SolrParams params = it.req.getParams(); 54: String cname = params.required().get(CoreAdminParams.CORE); 55: SolrCore core = it.handler.coreContainer.getCore(cname); 56: SolrQueryRequest wrappedReq = null; 57: 58: List<SolrCore> sourceCores = Lists.newArrayList(); 59: List<RefCounted<SolrIndexSearcher>> searchers = Lists.newArrayList(); 60: // stores readers created from indexDir param values 61: List<DirectoryReader> readersToBeClosed = Lists.newArrayList(); 62: Map<Directory, Boolean> dirsToBeReleased = new HashMap<>(); 63: 64: 65: if (core == null) return; {code} 2. {{CoreAdminOperation.execute() no longer needs to declare {{throws Exception}} since it always wraps any encountered exceptions with {{SolrException}}, which is unchecked. As a result, all implementing classes should also remove this declaration. (Note that this detail was included in this issue's description.) 3. Copy/paste-o's in {{CoreAdminOperationTest.java}}: 3.a. Should call {{REJOINLEADERELECTION_OP.execute()}} instead of {{REQUESTSTATUS_OP.execute()}} (and the failure message should be adjusted too): {code:java} public void testRejoinLeaderElectionUnexpectedFailuresResultIn500Exception() { final Throwable cause = new NullPointerException(); whenUnexpectedErrorOccursDuringCoreAdminOp(cause); try { CoreAdminOperation.REQUESTSTATUS_OP.execute(callInfo); fail("Expected request-status execution to fail with exception."); {code} 3.b. failure message prints "core-unload" instead of "core-reload": {code} public void testReloadUnexpectedFailuresResultIn500SolrException() { final Throwable cause = new NullPointerException(); whenUnexpectedErrorOccursDuringCoreAdminOp(cause); try { CoreAdminOperation.RELOAD_OP.execute(callInfo); fail("Expected core-unload execution to fail with exception."); {code} 3.c. failure message prints "request-sync" instead of "request-buffer-updates": {code} public void testRequestBufferUpdatesUnexpectedFailuresResultIn500Exception() { final Throwable cause = new NullPointerException(); whenUnexpectedErrorOccursDuringCoreAdminOp(cause); try { CoreAdminOperation.REQUESTBUFFERUPDATES_OP.execute(callInfo); fail("Expected request-sync execution to fail with exception."); {code} 3.d. failure message prints "request-apply-updates" instead of "request-status": {code} public void testRequestStatusMissingRequestIdParamResultsIn400SolrException() { whenCoreAdminOpHasParams(Maps.newHashMap()); try { CoreAdminOperation.REQUESTSTATUS_OP.execute(callInfo); fail("Expected request-apply-updates execution to fail when no 'requestid' param present"); {code} > Standardize response codes and success/failure determination for core-admin > api calls > ------------------------------------------------------------------------------------- > > Key: SOLR-11551 > URL: https://issues.apache.org/jira/browse/SOLR-11551 > Project: Solr > Issue Type: Bug > Security Level: Public(Default Security Level. Issues are Public) > Reporter: Varun Thacker > Assignee: Jason Gerlowski > Priority: Major > Attachments: SOLR-11551.patch, SOLR-11551.patch, SOLR-11551.patch > > > If we were to tackle SOLR-11526 I think we need to start fixing the core > admin api's first. > If we are relying on response codes I think we should make the following > change and fix all the APIs > {code} > interface CoreAdminOp { > void execute(CallInfo it) throws Exception; > } > {code} > To > {code} > interface CoreAdminOp { > /** > * > * @param it request/response object > * > * If the request is invalid throw a SolrException with > SolrException.ErrorCode.BAD_REQUEST ( 400 ) > * If the execution of the command fails throw a SolrException with > SolrException.ErrorCode.SERVER_ERROR ( 500 ) > * Add a "error-message" key to the response object with the exception ( > this part should be done at the caller of this method so that each operation > doesn't need to do the same thing ) > */ > void execute(CallInfo it); > } > {code} > cc [~gerlowskija] -- This message was sent by Atlassian JIRA (v7.6.3#76005) --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org