On Aug 15 2012, at 09:06 , Michael McMahon wrote:
>> HttpClientProvider::
>>
>> - HttpClientProvider JavaDoc uses inconsistent capitalization of HTTP.
>>
>> - createAsynchronousHttpClient() Since there's only one create method why
>> bother to mention that it's "Asynchronous"?
>>
>> - It's not clear how loadFromExternal() is different from provider().
>>
>> - provider enumeration, request alternate providers?
> Yes, the provider needs work. I agree with the above. Though I don't know
> about the
> necessity to provide multiple alternate providers.
If there is only ever going to be one provider then why include a provider
interface. I suspect that HttpClient will have about as many implementations as
there are JCE providers, ie. less than 10 but it is possible that someone may
have more than one simultaneously and want to select a particular provider.
>>
>>
>> HttpClient::
>>
>> - There's no way to identify the source of the HttpClient(). ie. it is
>> returned by provider() but there's no way to tell what you got. Would be
>> useful for debugging to log to the file what provider you received.
> I'm not sure I follow the comment above. Does it just relate to
> implementation rather than the API?
Yes. Mostly for diagnostic purposes. Since the provider isn't associated with
the JDK I need to be able to get version information about the provider to
record to logs, use in bug reports, etc.
>> - Why expose the connection cache? Seems like just a good way to mess things
>> up. Have you considered provider scope for connection cache? ie. all clients
>> from the same provider share a cache?
> The idea behind connection cache was to allow applications more control over
> opening, closing and caching of TCP
> connections. But, it's possibly not 100% right yet. I figured that its scope
> would be at client level. If there were a higher
> level notion of an application in Java SE, then it probably would be
> associated at that level imo.
> In what way do you think it would be easy to mess things up?
Failure to return connections mostly. Resource leakage.
>> - I missed the "s" in setHttpsConfigurator until I looked at the method in
>> more detail. It doesn't really stand out.
> maybe setSSLConfigurator() and change HttpsConfigurator to SSLConfigurator?
It would certainly stand out more though TLS is hopeful used.
>> - getFilters() -- this is the only modify-in-place API. Kinda weird. I think
>> having setFilters() would be better.
> Do you mean getFilters() returns a copy and setFilters() would just set the
> actual list?
Yes.
>> - setBody(Iterable<ByteBuffer>, boolean isRestartable) -- This seems
>> heinous. Non-restartable Iterables should be avoided. I like the suggestion
>> of additional setBody(Iterable<ByteBuffer>) method instead.
> Ok. So, a setBody(Iterator<ByteBuffer>) then ?
Yes.
>> - getBodyAsByteBuffers -- handling for not enough space in the array needs
>> to be documented. Unused entries nulled out?
> situation would only arise through ByteBuffer allocation failure, and
> presumably OOM exception would be thrown.
> Is it necessary to document this?
Probably not. OOM is an Error rather than an exception to it wouldn't be
reasonable for developers to try to catch it.
>> - Perhaps maxlength -> maxBytes to be more clear?
>>
>> - getBodyAsByteBufferSource -- if the same iterator is always returned why
>> not return the iterator rather than an iterable?
> The idea was to allow for use of the foreach convenience. eg
>
> Iterable<ByteBuffer> buffers = response.getBodyAsByteBufferSource();
> for (ByteBuffer buffer : buffers) {
> // handle data in buffer
> }
>
>
> This raises the same concern then about the Iterable being non-restartable,
> which in this case
> it clearly can't be. This is a "one-shot" streaming API. I notice that the
> documentation for
> Iterable is rather terse and doesn't say whether
> this usage is encouraged or discouraged. So, I'm not sure now.
One-shot Iterables are definitely discouraged
Mike
