You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by Ortwin Glück <or...@nose.ch> on 2002/11/28 20:45:10 UTC
Re: [PATCH] HttpClient/HttpMultiClient merge
I finally reviewed your patch: Woah, GREAT WORK!
Architecture looks clean. Tests run smoothly (except of 4 offline tests,
but this could be a config or a temporary problem).
There are a number of FIXME's. But this could easily cleaned up after
the commit.
I have some further enhancements but they do not block a commit:
- in MultiThreadedHttpConnectionManager (what a class name!), have you
considered using a WeakHashMap instead of building this whole weak ref
stuff yourself? This JDK 1.2 compatible. I did not look at the details
so maybe it is not possible.
- There are a lot of 'if (protocol.equalsIgnoreCase("HTTP")' statements.
We could make a Protocol class like:
public class Protocol {
private static Map protocols = new HashMap();
private String name;
private boolean secure;
static {
protocols.put("HTTP", new Protocol("http", false));
protocols.put("HTTPS", new Protocol("https", true));
}
private Protocol(String name, boolean secure) {
this.name = name;
this.secure = secure;
}
public static Protocol getProtocol(String name) {
return (Protocol) protocols.get(name);
}
}
In my opinion we can commit this.
(A patch that works with the current CVS HEAD would be nice...)
Odi
Re: [PATCH] HttpClient/HttpMultiClient merge
Posted by Oleg Kalnichevski <o....@dplanet.ch>.
On Thu, 2002-11-28 at 20:45, Ortwin Glück wrote:
> I finally reviewed your patch: Woah, GREAT WORK!
>
> Architecture looks clean. Tests run smoothly (except of 4 offline tests,
> but this could be a config or a temporary problem).
>
> There are a number of FIXME's. But this could easily cleaned up after
> the commit.
>
> I have some further enhancements but they do not block a commit:
>
> - in MultiThreadedHttpConnectionManager (what a class name!), have you
> considered using a WeakHashMap instead of building this whole weak ref
> stuff yourself? This JDK 1.2 compatible. I did not look at the details
> so maybe it is not possible.
>
> - There are a lot of 'if (protocol.equalsIgnoreCase("HTTP")' statements.
> We could make a Protocol class like:
>
> public class Protocol {
> private static Map protocols = new HashMap();
> private String name;
> private boolean secure;
>
> static {
> protocols.put("HTTP", new Protocol("http", false));
> protocols.put("HTTPS", new Protocol("https", true));
> }
>
> private Protocol(String name, boolean secure) {
> this.name = name;
> this.secure = secure;
> }
>
> public static Protocol getProtocol(String name) {
> return (Protocol) protocols.get(name);
> }
> }
>
>
> In my opinion we can commit this.
>
> (A patch that works with the current CVS HEAD would be nice...)
>
>
> Odi
>
>
> --
> To unsubscribe, e-mail: <ma...@jakarta.apache.org>
> For additional commands, e-mail: <ma...@jakarta.apache.org>
--
Oleg Kalnichevski <o....@dplanet.ch>
Re: [PATCH] HttpClient/HttpMultiClient merge
Posted by Ortwin Glück <or...@nose.ch>.
Oleg Kalnichevski wrote:
> Ortwin,
> I have already proposed similar kind of protocol registry a while ago.
Yes I saw that and I like it.
> I
> have already whipped up a few classes. If you decide to do it yourself,
> I think it should also include a default port and a socket factory.
That's okay. I am not too keen on implementing it myself, because you
already have a good solution. I am not sure anymore if your code
implements the Flyweight pattern to ensure there is only a single Object
per protocol.
Re: [PATCH] HttpClient/HttpMultiClient merge
Posted by Michael Becke <be...@u.washington.edu>.
Hello all. Sorry I haven't responded in a while. I'll have time
tomorrow to make the suggested changes and will post a new patch.
Mike
On Thursday, November 28, 2002, at 03:42 PM, Oleg Kalnichevski wrote:
> Oops. My hands seem to be shaky. Sorry about my previous post
>
> Ortwin,
> I have already proposed similar kind of protocol registry a while ago.
> I
> have already whipped up a few classes. If you decide to do it yourself,
> I think it should also include a default port and a socket factory. I
> do
> agree that Mike's design would look even cooler if we did away with
> that
> awkward isSecure flag in HttpClient and HttpConnection methods and all
> ugly code around it
>
> Cheers
>
> Oleg
>
>
>
> On Thu, 2002-11-28 at 20:45, Ortwin Glück wrote:
>> I finally reviewed your patch: Woah, GREAT WORK!
>>
>> Architecture looks clean. Tests run smoothly (except of 4 offline
>> tests,
>> but this could be a config or a temporary problem).
>>
>> There are a number of FIXME's. But this could easily cleaned up after
>> the commit.
>>
>> I have some further enhancements but they do not block a commit:
>>
>> - in MultiThreadedHttpConnectionManager (what a class name!), have you
>> considered using a WeakHashMap instead of building this whole weak ref
>> stuff yourself? This JDK 1.2 compatible. I did not look at the details
>> so maybe it is not possible.
>>
>> - There are a lot of 'if (protocol.equalsIgnoreCase("HTTP")'
>> statements.
>> We could make a Protocol class like:
>>
>> public class Protocol {
>> private static Map protocols = new HashMap();
>> private String name;
>> private boolean secure;
>>
>> static {
>> protocols.put("HTTP", new Protocol("http", false));
>> protocols.put("HTTPS", new Protocol("https", true));
>> }
>>
>> private Protocol(String name, boolean secure) {
>> this.name = name;
>> this.secure = secure;
>> }
>>
>> public static Protocol getProtocol(String name) {
>> return (Protocol) protocols.get(name);
>> }
>> }
>>
>>
>> In my opinion we can commit this.
>>
>> (A patch that works with the current CVS HEAD would be nice...)
>>
>>
>> Odi
>>
>>
>> --
>> To unsubscribe, e-mail:
>> <ma...@jakarta.apache.org>
>> For additional commands, e-mail:
>> <ma...@jakarta.apache.org>
> --
> Oleg Kalnichevski <o....@dplanet.ch>
>
>
> --
> To unsubscribe, e-mail:
> <ma...@jakarta.apache.org>
> For additional commands, e-mail:
> <ma...@jakarta.apache.org>
>
Re: [PATCH] HttpClient/HttpMultiClient merge
Posted by Oleg Kalnichevski <o....@dplanet.ch>.
Oops. My hands seem to be shaky. Sorry about my previous post
Ortwin,
I have already proposed similar kind of protocol registry a while ago. I
have already whipped up a few classes. If you decide to do it yourself,
I think it should also include a default port and a socket factory. I do
agree that Mike's design would look even cooler if we did away with that
awkward isSecure flag in HttpClient and HttpConnection methods and all
ugly code around it
Cheers
Oleg
On Thu, 2002-11-28 at 20:45, Ortwin Glück wrote:
> I finally reviewed your patch: Woah, GREAT WORK!
>
> Architecture looks clean. Tests run smoothly (except of 4 offline tests,
> but this could be a config or a temporary problem).
>
> There are a number of FIXME's. But this could easily cleaned up after
> the commit.
>
> I have some further enhancements but they do not block a commit:
>
> - in MultiThreadedHttpConnectionManager (what a class name!), have you
> considered using a WeakHashMap instead of building this whole weak ref
> stuff yourself? This JDK 1.2 compatible. I did not look at the details
> so maybe it is not possible.
>
> - There are a lot of 'if (protocol.equalsIgnoreCase("HTTP")' statements.
> We could make a Protocol class like:
>
> public class Protocol {
> private static Map protocols = new HashMap();
> private String name;
> private boolean secure;
>
> static {
> protocols.put("HTTP", new Protocol("http", false));
> protocols.put("HTTPS", new Protocol("https", true));
> }
>
> private Protocol(String name, boolean secure) {
> this.name = name;
> this.secure = secure;
> }
>
> public static Protocol getProtocol(String name) {
> return (Protocol) protocols.get(name);
> }
> }
>
>
> In my opinion we can commit this.
>
> (A patch that works with the current CVS HEAD would be nice...)
>
>
> Odi
>
>
> --
> To unsubscribe, e-mail: <ma...@jakarta.apache.org>
> For additional commands, e-mail: <ma...@jakarta.apache.org>
--
Oleg Kalnichevski <o....@dplanet.ch>
Re: [PATCH] HttpClient/HttpMultiClient merge
Posted by Ortwin Glück <or...@nose.ch>.
Michael Becke wrote:
> Agreed. The way protocol is handled is a bit nasty. Perhaps we should
> work on this after this initial patch is posed.
Sure. Some after-commit clean-up work is okay.
Re: [PATCH] HttpClient/HttpMultiClient merge
Posted by Michael Becke <be...@u.washington.edu>.
On Thursday, November 28, 2002, at 02:45 PM, Ortwin Glück wrote:
> I finally reviewed your patch: Woah, GREAT WORK!
>
> Architecture looks clean. Tests run smoothly (except of 4 offline
> tests, but this could be a config or a temporary problem).
>
> There are a number of FIXME's. But this could easily cleaned up after
> the commit.
>
> I have some further enhancements but they do not block a commit:
>
> - in MultiThreadedHttpConnectionManager (what a class name!), have you
> considered using a WeakHashMap instead of building this whole weak ref
> stuff yourself? This JDK 1.2 compatible. I did not look at the details
> so maybe it is not possible.
The WeakHashMap doesn't allow for the use of a ReferenceQueue. The
queue is required to determine when a weak reference is cleared.
> - There are a lot of 'if (protocol.equalsIgnoreCase("HTTP")'
> statements. We could make a Protocol class like:
>
> public class Protocol {
> private static Map protocols = new HashMap();
> private String name;
> private boolean secure;
>
> static {
> protocols.put("HTTP", new Protocol("http", false));
> protocols.put("HTTPS", new Protocol("https", true));
> }
>
> private Protocol(String name, boolean secure) {
> this.name = name;
> this.secure = secure;
> }
>
> public static Protocol getProtocol(String name) {
> return (Protocol) protocols.get(name);
> }
> }
Agreed. The way protocol is handled is a bit nasty. Perhaps we should
work on this after this initial patch is posed.
> In my opinion we can commit this.
>
> (A patch that works with the current CVS HEAD would be nice...)
>
>
> Odi
>
>
> --
> To unsubscribe, e-mail:
> <ma...@jakarta.apache.org>
> For additional commands, e-mail:
> <ma...@jakarta.apache.org>
>