[
https://issues.apache.org/jira/browse/SOLR-15629?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Chris M. Hostetter updated SOLR-15629:
--------------------------------------
Attachment: SOLR-15629.patch
Assignee: Chris M. Hostetter
Status: Open (was: Open)
{quote}a "lambda wrapper" (similar to how {{expectException(...)}} works) would
probably make more sense...
{quote}
I briefly toyed with this, but quickly realized it was actaully a big pain in
the ass, because there's no easy way for a method like this to propogate any
Throwable thrown by the wrapped lambda w/o just delcaring "throws Throwable"
(not an issue for {{expectException(...)}} because by design it *wants* to
catch exceptions and it *wants* to "fail" if a different type of exception is
thrown)
I realized what makes a lot more sense is a {{LogInterceptor implements
AutoClosable}} that sets up the log Filter in it's constructor/factory method,
and removes the interception in it's {{close()}} method – that way you can use
a "try-with-resources" to wrap the code you want to ignore exceptions in.
A Pro/Con of this approach is that you can call methods on the
{{LogInterceptor}} to "inspect" the intercepted log messages inside the try
block, w/o needing to wait until all of the logic in the block is done ... the
"Con" part being that you *MUST* call at least some method on the
{{LogInterceptor}} inside the try block, or the compiler warn/fails tha it's
unused. (Which i think is actually a "Pro": If a test wants to expect/ignore
some ERROR message logs, it should assert something about them)
The attached patch implements this idea – but I should note that I decided
pretty quickly that because of how log4j (and it's APIs) work, just having each
{{LogInterceptor}} add a {{Filter}} on the root logger ddin't seem like it
would really cut it if we also wanted to support "inspection"; because if we
wanted to have multiple {{LogInterceptor}} 's in use at the same time, each
interceptor/Filter could potentially get in each others way. It seems cleaner
to ask you what logger to "inspect" and use an Appender there with a Filter
that only ACCEPTs the LogEvents you are interested in, while also adding a
Filter to the ROOT logger to prevent those ERRORs from making it to the log
file.
The patch works, and you can see from the tests what it might look like if we
started using it – but the more i've been working on it, the more disatisfied
i've become with the both the surface API and a llot of the implementation
details.
I won't elaborate too much here in jira – it's pretty thoroughly spelled out in
nocommit comments in the patch, where i also flesh out what i think would be
better – for now i'll just point out:
* I do plan to keep working on this
* If i get hit by a bus and never finish this, please remember:
** a lot of complexity in the current patch (and in the suggestions in the
nocommit comments) comes from wanting to implement something *BETTER* then the
existing {{SolrTestCaseJ4.ignoreException}} functionality
*** I'd really like us to be able write better tests that are more strict
about if/when they expect an ERROR (or a WARN) to be logged
*** ideally even start failing the build if any test causes solr to log an
ERROR that isn't expected
** The original idea of just (temporarily) adding a Filter to the ROOT logger
(that DENY's an ERROR log matching the Pattern or (sub)String) is still very
viable and would be simple to implement as an alternative if someone is
interested.
> replace SolrException.ignorePatterns with a a new test-framework Rule
> ----------------------------------------------------------------------
>
> Key: SOLR-15629
> URL: https://issues.apache.org/jira/browse/SOLR-15629
> Project: Solr
> Issue Type: Improvement
> Reporter: Chris M. Hostetter
> Assignee: Chris M. Hostetter
> Priority: Major
> Attachments: SOLR-15629.patch
>
>
> We should deprecated & remove the following code:
> * {{SolrException.log()}}
> * {{SolrException.ignorePatterns}}
> * {{SolrTestCaseJ4.ignoreException()}}
> * {{SolrTestCaseJ4.unIgnoreException()}}
> * {{SolrTestCaseJ4.resetExceptionIgnores()}}
> ...and replace with something along the lines of ... (from SOLR-15628) ...
> {quote}We should probably re-think the entire existence of
> {{SolrException.log()}} and the API design of
> {{SolrException.ignorePatterns}} – replacing all callers of
> {{SolrException.log()}} with {{logger.error()}} and use a new test-only log4j
> Filter/Appdener
> {quote}
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
