Adds a factory for building ResponseProcessor.
#663
Conversation
| @@ -172,7 +176,8 @@ public DataServlet( | |||
| BroadcastChannel<String> preResponseStoredNotifications, | |||
| HttpResponseMaker httpResponseMaker, | |||
| ResponseFormatResolver formatResolver, | |||
| DataApiRequestFactory dataApiRequestFactory | |||
| DataApiRequestFactory dataApiRequestFactory, | |||
| ResponseProcessorFactory responseProcessorFactory | |||
There was a problem hiding this comment.
Since we are changing contract of this method and there are possibilities of extending DataServlet in downstream projects, shall we crate a new constructor and deprecate this one?
There was a problem hiding this comment.
We haven't been doing that for the DataServlet. And we could, but seeing as how
we always add something to this constructor whenever we need to add something
new to inject, DataServlet would very quickly become very noisy.
Also, although it is technically possible to override the DataServlet, I think
that's sufficiently weird and rare enough that having hordes of giant
deprecated constructors running around would do more harm than good.
Especially since this is an easy enough thing to fix.
| * @return A class that implements {@link ResponseProcessorFactory}. | ||
| */ | ||
| protected Class<? extends ResponseProcessorFactory> buildResponseProcessorFactory() { | ||
| return ResultSetResponseProcessorFactory.class; |
There was a problem hiding this comment.
Prob. not a blocker for this PR, but what if other ResponseProcessor implementations also need custom injection?
There was a problem hiding this comment.
Erm, that's the whole idea of this PR? If somebody wants to inject their
own custom ResponseProcessor then they:
- Define their
CustomResponseProcessor. - Define their
CustomResponseProcessorFactory. - Override this method and return
CustomResponseProcessorFactory.class
There was a problem hiding this comment.
Do you mean if they have non-empty constructors? Then they'd need to make their
constructor injectable so that HK2 can handle it.
archolewa
force-pushed
the
inject-response-processor
branch
from
c37997e
to
f4a6054
Compare
-- We would like to do a little bit of Druid error analysis to extract some more illuminating error codes out of some of Druid's 500's. The error callback is controlled by the `ResponseProcessor`. -- Unfortunately, the `ResponseProcessor` can't be injected directly because it depends on the `DataApiRequest`, which is built directly in each request. So instead we wrap the creation of `ResponseProcessor` in a factory that can be injected.
archolewa
force-pushed
the
inject-response-processor
branch
from
f4a6054
to
30450bb
Compare
-- We would like to do a little bit of Druid error analysis to extract
some more illuminating error codes out of some of Druid's 500's. The
error callback is controlled by the
ResponseProcessor.-- Unfortunately, the
ResponseProcessorcan't be injected directlybecause it depends on the
DataApiRequest, which is built directly ineach request. So instead we wrap the creation of
ResponseProcessorina factory that can be injected.