Roland Weber wrote:
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.
Hi Roland
At the moment I fail to see why request preprocessing and response
postprocessing could not be done by the dispatcher on a background
thread, but admittedly I have not studied the case in details, so I can
well be wrong here.
(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.
The problem is not about downcasting as such. It is rather about the
fact that the generic HttpDispatcher interface may not always be usable
unless cast to a concrete type.
(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.
Overall, I think the code is good enough to be imported into SVN as is
(probably with System.out.println() and Throwable#printStackTrace()
problem fixed). You should go ahead and add more concrete dispatchers.
With several dispatchers in place it will be easier to identify
commonalities in functionality (leading to better interfaces) and those
in code (leading to a better code reuse)
Oleg
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
