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 20:29:19 GMT
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.

Michael

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


Mime
View raw message