hc-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Osipov <micha...@apache.org>
Subject Re: HC 5.0: HttpCore 5.0 alpha1 release
Date Tue, 01 Dec 2015 21:17:41 GMT
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?

> 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?

Michael

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


Mime
View raw message