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: [email protected] For additional commands, e-mail: [email protected]
