[
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: [email protected]
For additional commands, e-mail: [email protected]
