[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=16411887#comment-16411887
]
Jason Gerlowski edited comment on SOLR-11551 at 3/23/18 6:53 PM:
-
Man, thanks for the thorough review Steve. Those copy-paste issues are tough
for familiar eyes to catch.
I've attached a patch which fixes concerns (1) and (3) above.
I disagree a bit with concern (2) though. Or rather, the code does. With the
wrap-all-exceptions-but-SolrException logic pulled into
{{CoreAdminOperation.execute()}}, many of the CoreAdminOp implementations still
throw checked exceptions. (Most commonly IOException and
InterruptedException). See CreateSnapshoOp, DeleteSnapshotOp, MergeIndexesOp,
PrepRecoveryOp, RestoreCoreOp, SplitOp, and StatusOp for examples. Removing
{{throws Exception}} from the interface declaration was one of the sacrifices
made when unifying all of the nearly identical exception-wrapping bits.
I'm happy to walk back the try-catch unification if you'd rather see the
{{throws Exception}} clause gone from the interface. There might be some other
changes with these types that can get us around this too. Haven't had much
time to look yet. But I'm not sure what they would buy us.
My slight preference though would be to leave things as they are. Each
implementation is free to throw non-SolrExceptions, but we have a single place
that inspects and optionally repackages any exceptions that trickle out. But
I'm happy to defer if you feel strongly about getting that {{throws Exception}}
out of CoreAdminOp.
was (Author: gerlowskija):
Man, thanks for the thorough review Steve. Those copy-paste issues are tough
for familiar eyes to catch.
I've attached a patch which fixes concerns (1) and (3) above.
I disagree a bit with concern (2) though. Or rather, the code does. With the
wrap-all-exceptions-but-SolrException logic pulled into
{{CoreAdminOperation.execute()}}, many of the CoreAdminOp implementations still
throw checked exceptions. (Most commonly IOException and
InterruptedException). See CreateSnapshoOp, DeleteSnapshotOp, MergeIndexesOp,
PrepRecoveryOp, RestoreCoreOp, SplitOp, and StatusOp for examples. So removing
the {{throws Exception}} from the interface declaration was one of the
sacrifices made to unify all of the nearly-identical-exception-wrapping bits.
I'm happy to walk back the try-catch unification if you'd rather see the
{{throws Exception}} clause gone from the interface. There might be some other
changes with these types that can get us around this too. Haven't had much
time to look yet. But I'm not sure what they would buy us.
My slight preference would be to leave the error logic as-is. Each
implementation is free to throw non-SolrExceptions, but we have a single place
that inspects and optionally repackages anything that gets thrown. But I'm
happy to defer if you'd prefer a different plan of attack here.
> 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,
> 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
[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=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 sourceCores = Lists.newArrayList();
59:List searchers = Lists.newArrayList();
60:// stores readers created from indexDir param values
61:List readersToBeClosed = Lists.newArrayList();
62:Map 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 sourceCores = Lists.newArrayList();
59:List searchers = Lists.newArrayList();
60:// stores readers created from indexDir param values
61:List readersToBeClosed = Lists.newArrayList();
62:Map 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}}:
[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=16240187#comment-16240187
]
Jason Gerlowski edited comment on SOLR-11551 at 11/6/17 12:06 PM:
--
I also noticed that anytime a core-API takes in a "core" parameter, but that
core doesn't exist, we throw a SolrException with a 400 status-code. This
might be better represented as a 404 (NOT FOUND). I don't have a strong
opinion whether we implement that, but it does fit things a little maybe.
I left the behavior as-is in the initial patch, since it wasn't in the
interface you laid out in the issue description. Just wanted to mention it in
case anyone liked the idea...
was (Author: gerlowskija):
I also noticed that anytime a core-API takes in a "core" parameter, but that
core doesn't exist, we throw a SolrException with a 400 status-code. This
might be better represented as a 404 (NOT FOUND). I don't have a strong
opinion whether we implement that, but it does fit things a little maybe.
> 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
> Attachments: 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
(v6.4.14#64029)
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
