lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Steve Rowe (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (SOLR-11551) Standardize response codes and success/failure determination for core-admin api calls
Date Fri, 23 Mar 2018 01:47:00 GMT

    [ https://issues.apache.org/jira/browse/SOLR-11551?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16410678#comment-16410678
] 

Steve Rowe commented on SOLR-11551:
-----------------------------------

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


Mime
View raw message