Hi Oleg,
thanks for taking the time to review the code and to provide your feedback.
See my comments inline.
> (1) AsyncHttpProcessor
>
> Do we really need all these fine-grained methods? Can't #finishResponse() be
> merged into #obtainResponse() and #prepareRequest() into #transmitRequest()?
No, they can't. prepareRequest and finishResponse are executed by
application threads, while transmitRequest and obtainResponse are
executed by the background thread. The split of responsibilities
between dispatcher background threads and application threads is
IMHO one of the most crucial points in the design of http-async.
> Ideally I would prefer *HttpProcessor classes to not expose HttpMutable*
> objects
> to the caller
>
> Do we need this class at all?
I took the code from HttpRequestExecutor and refactored it in order
to get the finer-grained methods I need for dispatching. For the time
being, I kept it in a separate processor class as an indication of
the origin. Since all of the actual functionality is duplicated from
HttpRequestExecutor, I would like to work with you on refactoring
HttpRequestExecutor such that it can be used for http-async, too.
Then, AsyncRequestProcessor becomes obsolete or is reduced to a small
adapter class derived from HttpRequestProcessor. What remains can be
turned into a dispatcher base class, then the methods don't have to be
public anymore.
> (2) SimpleHttpHandle
>
> if (response_object != null) {
> // postprocessing is delegated back to the dispatcher
> ((SimpleHttpDispatcher) http_dispatcher)
> .postprocessResponse(this, response_object);
> }
>
> Having to cast an interface to a concrete class in order to get some work done
> is usually a bad sign suggesting that the API is not flexible enough
The dispatcher reference is stored in the abstract base class and has
therefore the generic interface type. The reference is passed to the
constructor with concrete type SimpleHttpDispatcher. I asked myself if it
is worth to keep the same reference in two different attributes just to
avoid downcasting, and decided to go with the casts since the signature
of the constructor guarantees the type to which I cast. If you prefer
that, I can add the duplicate attribute and remove the casts. It would
save runtime type checks, too.
> (3) SimpleHttpDispatcher
>
> Somehow I can't help thinking that functionality of AsyncHttpProcessor
> actually
> belongs to SimpleHttpDispatcher. Consider moving all the code from
> AsyncHttpProcessor to SimpleHttpDispatcher.
SimpleHttpDispatcher is the minimal proof-of-concept implementation of a
dispatcher. I expect to implement at least two more dispatchers, one using
a lot of threads and traditional IO, and another one using less threads
and NIO. The functionality in AsyncHttpProcessor will be needed by all
three dispatchers. But see my comment above on the future of that class.
> (4) System.out.println
>
> There should be no calls to System.out.println and
> Throwable#printStackTrace().
> If you can't just rethrow exceptions and/or absolutely have to print traces,
> consider using a logging toolkit
I will. That's one of the reasons why I rate the code as having only
preview quality. The @@@ lines are displayed by my editor in bold red :-)
The decision about the logging toolkit for 4.0 is still pending though:
http://issues.apache.org/bugzilla/show_bug.cgi?id=32937
> (5) Overall it is a good start. The quality of javadocs in your code puts me
> to
> shame. I have been working on HttpCore for almost a year now, however the
> quality of its javadocs is nowhere near that of HttpAsync.
Thanks a lot. I have trained myself to write at least the method description
along with the signature, before even starting to implement anything :-)
I'll help write the JavaDocs for http-core once I've become sufficiently
familiar with it. Currently each dive into the code leads to surprises,
so I'm not yet up to it.
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
