You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucene.apache.org by "Steve Rowe (JIRA)" <ji...@apache.org> on 2018/03/23 01:49:00 UTC
[jira] [Comment Edited] (SOLR-11551) Standardize response codes and
success/failure determination for core-admin api calls
[ 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-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org