hc-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Oleg Kalnichevski <ol...@apache.org>
Subject Re: HC 5.0: HttpCore 5.0 alpha1 release
Date Tue, 01 Dec 2015 22:44:29 GMT
On Tue, 2015-12-01 at 22:17 +0100, Michael Osipov wrote:
> Am 2015-12-01 um 21:52 schrieb Oleg Kalnichevski:
> > On Tue, 2015-12-01 at 21:50 +0100, Michael Osipov wrote:
> >> Am 2015-12-01 um 21:46 schrieb Oleg Kalnichevski:
> >>> On Tue, 2015-12-01 at 21:29 +0100, Michael Osipov wrote:
> >>>> Am 2015-12-01 um 14:35 schrieb Oleg Kalnichevski:
> >>>>> Folks
> >>>>>
> >>>>> I would like to start cutting RC1 for HttpCore 5.0 alpha1 sometime
soon.
> >>>>>
> >>>>> Could you please take a look at the latest snapshot and let me know
if
> >>>>> you find anything that might block the release or cause another
RC
> >>>>> build?
> >>>>
> >>>> While this is an alpha release, the code does not show overall good
> >>>> fitness. It will take a look of tweaks for GA.
> >>>>
> >>>> I've made a quick code review and here are my findings:
> >>>>
> >>>> 1. What about the artifact ids? Didn't we want to apply the same scheme
> >>>> as Apache Commons?
> >>>>       httpcore5-parent
> >>>>       |- httpcore5
> >>>>       |- httpcore5-ab
> >>>>       etc..
> >>>>
> >>>> 2. httpcore/src/main/appended-resources/META-INF/...
> >>>>       This can be autogenerated with
> >>>> https://maven.apache.org/apache-resource-bundles/
> >>>>
> >>>> 3. version.properties requires a relocation and an update
> >>>>
> >>>> 4. Remove all not used Subversion properties like Id, Author, etc. but
> >>>> not eol-style of course.
> >>>>
> >>>> 5. How far are the imported annotations used throughout the code? Are
> >>>> the annotated classed analyzed at runtime or do they serve mere
> >>>> documentation which Javadoc could do?
> >>>>
> >>>> 6. Rename "io" packages to "bio" to make distinction from "nio" (uniform
> >>>> naming)
> >>>>
> >>>> 7. Several comments still refer to the old RFC. Javadoc need to be
> >>>> proof-read and updated to the new RFCs.
> >>>>
> >>>> 8. HttpResponse throws IllegalStageException for illegal arguments.
Weird?!
> >>>>
> >>>> 9. Buffer sizes vary from 2 KiB to 8 KiB throughout the code. What about
> >>>> normalization?
> >>>>
> >>>> 10. ContentType: character encoding of application/*-xml is set to
> >>>> ISO-8859-1 but the default encoding for XML is UTF-8.
> >>>> (I am ignoring the XML prolog for now)
> >>>>
> >>>> 11. HeaderGroup: Using a get(int), set(int, Object) on a list (here
> >>>> List<Header>) might incur a performance penalty because they were
not
> >>>> designed for that.
> >>>>
> >>>> 12. TokenParser duplicates final statics from Chars
> >>>>
> >>>> 13. Several methods throughout the code declare throws
> >>>> DerivedFromRuntimeException" this is code smell and needs to be removed.
> >>>> Unchecked exception appear only in Javadocs.
> >>>>
> >>>> 14. HttpDateGenerator is marked as @ThreadSafe but SimpleDateFormat
is
> >>>> not. WTF?
> >>>>
> >>>> 15. Several wellknown literals are used where statics would be
> >>>> appropriate like 80, 443, HTTP methods, "http", "https" etc.
> >>>>
> >>>> 16. System.currentTimeMillis() is used at several spots where elapsed
> >>>> time is measured. This approach is obsolete/error-prone and shall
> >>>> replaced with System.nanoTime()
> >>>>
> >>>> 17. ByteBufferAllocator and its implementors seem to be pretty much
> >>>> useless/overdesigned just to wrap one method: allocate/allocateDirect,
> >>>> aren't they?
> >>>>
> >>>> 18. Replace Args, Asserts, TextUtils, LangUtils, SimpleDateFormat (#14)
> >>>> with Commons Lang. There is so much duplicate code that it is worth
> >>>> having Commons Lang 3 as dependency.
> >>>>
> >>>> 19. What I absolutely do not like/understand is the inconsistent
> >>>> approach of custom exceptions (not a flame war here please):
> >>>>        * At several points default exceptions are used inconsistently
or
> >>>> do not even make sense
> >>>>        * There is no clear distinction between recoverable and
> >>>> unrecoverable exceptions, e.g.,
> >>>>          * why do I have catch a ParseException where I cannot do anything
> >>>> about it?
> >>>>          * I cannot retry anyway when there is malformed input.
> >>>>     * Server sends an HTTP message not conforming with the RFCs, I cannot
> >>>> do anything about it in my code either.
> >>>>
> >>>>       The exceptions require a major overhaul in this new major version.
> >>>>
> >>>> I would work on them piece by piece if no one objects. If I need some
> >>>> guidance, I will ask separately on the list. Feel free to comment on
> >>>> those but please delete the unnecessary parts of this mail.
> >>>>
> >>>
> >>> I _very_ much object to 13, 14, 16 to 19, _especially_ 18 and 19. I am
> >>> not in favor of point 6.
> >>
> >> Can you please elaborate on your objection?
> >>
> >
> > Of course, I can. Some points, like point 14, are just plain wrong.
> 
> Are you claiming that SimpleDateFormat is threadsafe although it is 
> documented not to be? Or is it synchronized externally?
> 

Please have a look at how the class is implemented. It even has
annotations specifically to document its thread-safety guarantees.

> > Others are highly disputable like 18 and 19. Let's concentrate on things
> > that objectively need fixing.
> 
> You wrote several times that the code has not much changed in 3.x and 
> 4.x. No substantial improvements or cleanups have been made for years. 
> Things which may have been correct years ago aren't necessarily now.
> 
> What is the point relocating stuff without improving it? I fully 
> understand that you want to focus on HTTP/2 but given that you are the 
> only real committer on this project (except the work Gary and Sebb are 
> making) any type of help should be given its appreciation and especially 
> that so many people are using HttpClient but give so little contribution 
> back.
> 
> Why should we reinvent the wheel and reimplement others have already 
> done at ASF?
> When I wrote the very first code at work with HttpCore and HttpClient 
> the exceptions did not feel right and were not easy to understand. Why 
> can't we make improvements here?
> 

I very much disagree about adding an extra dependency as being an
improvement for no reason other than personal taste. Same goes for
exception handling, which is another very subjective matter. I want to
suggest to focus on solving real issues, like Kerberos support in
HttpClient, not discourage you from contributing. Given our very limited
resources I want to suggest that we stay focused on things that matter
more.  

Oleg 


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org


Mime
View raw message