[
https://issues.apache.org/jira/browse/SOLR-11551?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=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: [email protected]
For additional commands, e-mail: [email protected]
