[
https://issues.apache.org/jira/browse/SOLR-18236?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18080398#comment-18080398
]
Chris M. Hostetter commented on SOLR-18236:
-------------------------------------------
On the whole this seems like a good idea ... some more specific thoughts below
in no particular order.
One thing I will call out is that I'm going to try to avoid commenting on
specifics of specifics of if/when/how to break back compact in order to avoid
implying that have strongly held convictions for any weak opinions.
----
{quote}SolrServerException — client-side orchestration / pre-flight failure
{quote}
If we're going to make a lot of changes to the exception hierarchy, and we're
going to clearly and explicitly define this exception type to be client-side
related problems – then now is probably a good time to rename this exception?
Thinking about this lead me to realize something: I haven't done in depth code
base archeology to verify this, but i'm pretty sure the name
"SolrServerException" dates back to when the "client abstraction" objects were
named "SolrServer" (ie: "EmbeddedSolrServer" vs "HttpSolrServer") – before
being renamed "SolrClient".
In other words: I don't think the name "SolrServerException" was ever intended
to convey "The Solr Server generated an Exception" I think it was intended to
convey that "The SolrServer generated an Exception"
It seems like a historical oversight that the "SolrServer -> SolrClient"
renaming didn't also include a "SolrServerException -> SolrClientExceptino"
----
These two statements seem at odds with each other...
{quote}SolrServerException — client-side orchestration / pre-flight failure
Failures that happen on the client side, before or instead of a successful HTTP
exchange:
...
SolrServerException — add a constructor accepting a cause Throwable plus HTTP
code (so RemoteSolrException can chain through).
{quote}
Based on the proposed vision, I don't see any reason why SolrServerException
should have any explicit knowledge of HTTP codes?
if RemoteSolrException is re-parented to "extends SolrServerException" and thus
loses it's it's inherited HTTP concepts & methods from SolrException, then it
seems like the "fix" is to add those concepts & methods directly to the
RemoteSolrException class – why should it's (new) parent class
SolrServerException be changed to accommodate it?
----
This phrasing captured my imagination and gave me pause...
{quote}SolrException — the shared HTTP-shaped error type
{quote}
...the half formed thought that formed during that pause is something along the
lines of:
* Assuming:
** we are re-parenting RemoteSolrException from SolrException to
SolrServerException
** but RemoteSolrException still needs to track the same HTTP "concepts" as
SolrException
* Then:
** Would it make sense for RemoteSolrException to be _composed_ of a
SolrException (instead of subclassing it)
** Would it make migration for clients any easier if there was a
"rethrowAsUncheckedSolrException()" type method on RemoteSolrException?
Something like...
{code:java}
public final class RemoteSolrException extends SolrServerException {
private final SolrException raw;
public RemoteSolrException(String remoteHost, int code, String msg, Throwable
th) {
super("Error from server at " + remoteHost + ": " + msg, th);
this.raw = new SolrException(code, this.getMessage(), this);
}
public void rethrowAsUncheckedSolrException() {
throw raw;
}
// ... more public methods like code() that just pass through to raw.code(),
etc...
{code}
----
Going back to these vision statements...
{quote}SolrServerException — client-side orchestration / pre-flight failure
Failures that happen on the client side, before or instead of a successful HTTP
exchange:
...
RemoteSolrException — server replied with an HTTP error
Re-parented under SolrServerException. Becomes checked.
{quote}
I start to wonder if it actually makes conceptual sense for "The exception type
that indicates a server responded with an HTTP error" to *EXTEND* "The
exception type that represents client-side orchestration / pre-flight failure"
.... or are we only considering that because SolrServerException is a checked
exception that most of our APIs already throw?
Combining this line of thinking with my question about *_renaming_*
SolrServerException -> SolrClientException, I start to wonder if instead what
we should really be talking about is *_deprecating_* SolrServerException, and
using it only as an (effectively) abstract base class for two final subclasses:
(new) SolrClientException and a (reparented) RemoteSolrException?
{noformat}
Throwable
├─ Exception (checked)
│ ├─ IOException (transport)
│ └─ SolrServerException (deprecated, any existing 'new' or
'throws' are replaced with SolrClientException)
│ ├─ SolrClientException (client orchestration)
│ └─ RemoteSolrException (server returned HTTP error)
└─ RuntimeException
└─ SolrException (shared error w/ HTTP-code shape)
{noformat}
Resulting SolrClient.request signature:
{code:java}
public abstract NamedList<Object> request(SolrRequest<?> req, String coll)
throws SolrClientException, RemoteSolrException, IOException;
{code}
But anybody currently using {{catch (SolrServerException e)}} should still be
fine (until the deprecated SolrServerException is eventually {_}removed{_})
Alternatively, if we think we'll always want some sort of "common parent" for
SolrClientException and RemoteSolrException, then we could use something like...
{noformat}
Throwable
├─ Exception (checked)
│ ├─ IOException (transport)
│ └─ SolrServerException (deprecated, any existing 'new' or
'throws' are replaced with SolrClientException)
│ └─ SolrJException (new abstract base class)
│ ├─ SolrClientException (client orchestration)
│ └─ RemoteSolrException (server returned HTTP error)
└─ RuntimeException
└─ SolrException (shared error w/ HTTP-code shape)
{noformat}
* All existing method signatures that "throws SolrServerException" are
replaced with "throws SolrJException"
* When SolrServerException is eventually removed, SolrJException is changed to
"extends Exception" directly
> SolrJ exception hiearchy needs work
> -----------------------------------
>
> Key: SOLR-18236
> URL: https://issues.apache.org/jira/browse/SOLR-18236
> Project: Solr
> Issue Type: Improvement
> Components: SolrJ
> Reporter: David Smiley
> Priority: Major
>
> +SolrClient.request()+ declares +throws SolrServerException, IOException+
> with javadoc claiming:
> * *IOException* — "communication error with the server"
> * *SolrServerException* — "error on the server"
> Reality is the inverse on both axes:
> ||Scenario||Actual exception||Checked?||
> |Server returns non-2xx|RemoteSolrException (extends SolrException →
> RuntimeException)|*unchecked*|
> |Response body read fails (true I/O error)|RemoteSolrException|*unchecked*|
> |Server timeout / connection refused|SolrServerException (wraps the
> IOException)|checked|
> |No live servers / retries exhausted|SolrServerException|checked|
> |Raw IOException propagated as-is|almost never|—|
> h3. Contradictions
> # "Error on the server" (4xx/5xx reply) surfaces as *unchecked*
> RemoteSolrException — the javadoc says it should be SolrServerException.
> Compiler cannot help callers handle the most common failure mode.
> # "Communication error" rarely surfaces as IOException. Every concrete
> SolrClient wraps IOException (ConnectException, socket timeout, etc.) into
> SolrServerException. The +throws IOException+ clause is essentially dead.
> # HttpSolrClientBase.processErrorsAndResponse catches a real I/O error
> reading the response body and re-throws it as RemoteSolrException — calling a
> transport failure a "remote error."
> # 52 call sites just do +catch (SolrServerException | IOException e)+ and
> treat them identically — the split has no observable value at most call sites.
> # Yet 45 sites specifically catch RemoteSolrException to inspect +e.code()+
> — the HTTP-status-bearing exception is the genuinely useful concept, and it
> is the one the type system hides.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
