Costin Manolache wrote:
For a separate opinion:

In the trunk version:
- the '...' and array return seem strange and generate GC ( not a big issue
those days, but still inconsistent with the
rest of tomcat )
yes, its a new language feature, hence it wasn't available in previous JDKs or Tomcat. its a vararg, if we don't want it we can switch to arrays, it works the same way.

- the API seems a bit over-complex - for example, why
setConfiguration(COMMET_BLOCKING) instead of setBlocking() ?
I agree that it less likely to break implementations by adding to the enom
instead of adding methods to the interface - not sure
why it's not an abstract class in the first place.
if we want to simplify it, we would call it
configureBlocking(boolean) - consistent with JDK APIs :)

- please don't call the method configure(), it's commonly used with a
different meaning ( i.e. setting the port or general configuration).
setConnectionMode, etc. And using the enum doesn't sound consistent with
other APIs either.
we can call it whatever we want. But saying not using enum, its not consistent with other APIs in Tomcat, means would never take advantage of new language features ever, I think that would be a shame.

- see bellow - I don't think I understand the benefits of mixing blocking
and non-blocking in this interface, it is quite confusing.
It would be mixing it, its a one time config, during the BEGIN event, you say
configureBlocking(true) or configureBlocking(false).
Comet is very much connection centric, so you can't mix it.

In the trunk API, its clear to what you are using, blocking or non blocking, in the sandbox API, the swap of it happens when invoking isWriteable or isReadable, making the state of the comet connection confusing to the developer.

Filip

In the sandbox version:
- sleep() and setTimeout(int) -> why not sleep(int millis) ? I think it's
confusing to have both and the interactions between them, in
particular setTimeout is marked optional ? It makes sense to have
setTimeout() as a general timeout.

- not sure I understand the use case for isReadable()/isWriteable() - when
will this be called ? My understanding was that when
the connection is readable, a callback will be generated with EventType ==
READ. Also it's very confusing to mix the 'blocking' in the
isReadable()/isWriteable() - it would be much cleaner to say that the
connection is always non-blocking, and add a method to switch to blocking
mode ( then use the regular servlet methods maybe ). Using the ComentEvent
for both is IMHO dangerous.

- will sleep() still allow callbacks ( and if not - what will happen with
them )? What's going to happen with callbacks if callback() is not called ?


In general ( both versions):
- it would be great to move it from o.a.catalina to org.apache.comet

Costin

On 6/11/07, Remy Maucherat <[EMAIL PROTECTED]> wrote:

Filip Hanik - Dev Lists wrote:
> Ok, let me see if I can summarize.
>
> 1. Whether you write out the stored buffer using the Poller thread, or a > Tomcat worker thread (flushed in Http11xxxProcessor) as described below
> I originally thought of this as async write, as we are simply doing a
> write with another one of our threads. Originally when we were talking
> non blocking writes, I was thinking along the lines of non blocking to
> where the Comet developer had to do that logic, just as he was writing a
> socket, possibly like (but not suggested) a
> CometEvent.nonBlockWrite(ByteBuffer).
>
> 2. Do we need non blocking? with the methods of isWriteable and the
> ability to register(OP_WRITE)->event(WRITE), if the number of bytes you
> write is usually smaller than the socket buffer, chances are that most
> writes will be non blocking. I would even argue a large majority would
> be non blocking, and thus the implementation or the complexity thereof
> would not be needed. And with the ability to do async writes, means I
> can create my own thread pool/write queue to perform these writes.

You are writing the opposite thing to the previous email, and we are
back to "non blocking is useless". The problem is that I understand
blocking IO as "write this data, and return when it's done". If the
socket is in blocking mode, any write done by the servlet may block,
regardless of what isWriteable says. Of course, it's very unlikely,
which is why Comet in 6.0.x works.

> 3. isWriteable - simple method, but I don't like that the method in
> itself performs actions like adding the socket to a poller etc.
>   Instead isWriteable==true means that you can write on the socket,
> isWriteable==false you cannot. This method should be able to be invoked > as many times as its wanted, and is thread safe and doesn't do anything
> funky underneath.

Ok, so you prefer a more complex API (if I follow "just in case it was
useful"). I started with an API which would expose all operations, and
looked into removing what was not explicitly useful.

> 4. isWriteable - I'm also reading in that you are also suggesting that
> we use this method to declare if we want blocking or non blocking
writes.

No. The situation where write could (maybe) block is if the servlet
writes in a Tomcat thread. Typically, this is the reply-later design,
using the sleep/callback methods. The isWriteable method is not used,
since the servlet merely wants (in that common design) to send a
response as fast as possible, and typically this sort of response is not
too large and unlikely to cause IO problems. This blocking behavior is
allowed in that case to avoid forcing the user to put in more complex
logic to deal with the partial write + event, and is set just for the
amount of time it takes to perform the write (note that this ).

>   At this point this method is doing three things:
>   a) returns true/false if we can write data
>   b) delegates a socket to the poller to write data and generate a
> event(WRITE) to the comet processor
>   c) configures a write to be blocking or non blocking
> This is for sure not what I would expect of a "simple API", if simple
> means less keystrokes than yes, but simple to me also means intuitive
> and easily understood.

So you have plenty of methods to do the same thing.

> Given points 1-4, this is what is going to happen to every single
developer
>  I) They are going to use stream.write and event.isWriteable all the
> time, not realizing what it actually does
> II) They are going to get confused when they receive an IOException for
> trying to perform a write, cause they used isWriteable and the socket
> went into non blocking mode

If that's what you want to believe ...

> At this point, this 'simple' API, obviously not so simple, instead it
> becomes very complicated, as I would almost have to reverse engineer the
> code to truly understand what it does.
> It may be simple to you and me, but that is because we are implementing
it.

I really don't see what is complex, especially when you look at the code
the user would write for the simple cases, where you don't even have to
use any API besides stream.write:
- reply later
- wait for read events, and write data in response to it

The complex case deals with handling incomplete async writes if you
don't simply drop connection.

> so what does this mean to 'isReadable'? That I'm automatically
> registering for a READ event if it returns false? Maybe I don't want a
> READ event, I just want to see if any data has trickled in. so if I call

> sleep(), should I then call isReadable() to reregister for the read. how
> is this simpler than that register/unregister.

Read events always occur, unless you use sleep/callback. If this is not
written clearly in the javadocs already, I need to change them.

> Where does that leave us, well:
> a) We are almost in sync on the implementation side of it

Not really, there's a big disconnect in the understanding of non
blocking vs blocking, and according to you, non blocking is not useful
(again).

> b) I believe your API is not intuitive, nor usable, as it simply doesn't
> reflect what is going on and/or what a programmer wants done
> c) Your API doesn't become simpler just cause we merge three methods
> into one -> configure(NON_BLOCK), write(byte[]), register(OP_WRITE)
> c) The API I propose, you think is overengineered, I think it just makes
> things much clearer, the fact that it automatically allows for future
> extension is a side effect rather than design decision

My API is simpler because the code the user has to write is more
straightforward and easier to understand. Feel free to write small
examples to see for yourself.

> So bottom line is, user will get the same implementation (or very close > to what we've talked about), question is what API are they going to get?

Rémy

---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]


------------------------------------------------------------------------

Internal Virus Database is out-of-date.
Checked by AVG Free Edition. Version: 7.5.472 / Virus Database: 269.8.11/837 - Release Date: 6/6/2007 2:03 PM


---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

Reply via email to