cometd 2.0

[Greg Wilkins]

All,

Simone and I have started working on cometd 2.0, which is a refactor of the
code base of cometd1.0 with the two purposes of:

+ cleaning up dead end ideas that hang over from the development of
cometd and bayeux
+ to support websocket and other new transports.

The branch for this is https://svn.cometd.org/branches/cometd2
and it will be HIGHLY experimental and unstable for some time.


Simone,

I'm not sure I like the changed package structure.

The API has moved from org.cometd.* to bayeux.*

While org.bayeux.* might be workable, bayeux.* is not a
standard package name. Failing a perfect alternative,
I think that keeping org.cometd.* is probably best.
Maybe org.cometd.bayeux.* for the api


You client work is in org.cometd.bayeux.client*
I think that is a bit tautological - as cometd is a project
that implements bayeux. You might have a org.acme.bayeux
if some other organization does a bayeux implementation,
but org.comet.bayeux is just like saying org.bayeux.bayeux.

So again, unless it is clearly better, I advocate minimal
change to the packaging - something like:

org.cometd.bayeux = API
org.cometd.bayeux.client = Client API
org.cometd.bayeux.server = Server API

org.cometd.server = server impl
org.cometd.client = client impl



For naming within the client, I think we need to be careful with
the use of the work "client" for any entities. This will
be good for getting away from the confusion of server-side
clients vs client side clients.

I think the top level interfaces for the key concepts
should just be for identity and not for usage:


package org.cometd.bayeux;

interface Client
{
String getClientId()
}

interface Channel
{
String getChannelId();
}

interface Subscription extends Channel
{
void unsubscribe();
}


Then the client API can be:

package org.cometd.bayeux.client;

interface Bayeux
{
Session handshake(...);
}

interface Session extends Client
{
ClientChannel getClientChannel(String channel);
void disconnect();
}

interface ClientChannel extends Client,Channel
{
void publish(Object msg);
Subscription subscribe(MessageListener listener);
}



And on the server we can have

package org.cometd.bayeux.server;

interface Bayeux
{
LocalSession newLocalSession(String idBase);
addListener(BayeListener listener);
ServerChannel getChannel(String channelId);
}

public interface ClientBayeuxListener extends BayeuxListener
{
public void clientAdded(Session client);
public void clientRemoved(Session client);
}

interface Session extends Client
{
void deliver(msg);
addMessageListener(MessageListener listener);
void disconnect();
}

interface LocalSession extends Session, org.cometd.bayeux.client.Session
{
}

interface ServerChannel extends Channel
{
boolean isLazy();
boolean isPersistent();
boolean isBroadcast(); // !meta and !service

Set<Session> getSubscribers();
void addDataFilter(DataFilter filter);
void addListener(ChannelListener listener);
}



cheers

[Simone Bordet]

Hi,

On Wed, Dec 9, 2009 at 05:41, Greg Wilkins <gr...@intalio.com> wrote:
> I'm not sure I like the changed package structure.
>
> The API has moved from org.cometd.* to bayeux.*
>
> While org.bayeux.* might be workable,  bayeux.* is not a
> standard package name.   Failing a perfect alternative,
> I think that keeping org.cometd.* is probably best.
> Maybe org.cometd.bayeux.* for the api

Agreed.

> You client work is in org.cometd.bayeux.client*
> I think that is a bit tautological - as cometd is a project
> that implements bayeux.   You might have a org.acme.bayeux
> if some other organization does a bayeux implementation,
> but org.comet.bayeux is just like saying org.bayeux.bayeux.
>
> So again, unless it is clearly better, I advocate minimal
> change to the packaging - something like:
>
>  org.cometd.bayeux = API
>  org.cometd.bayeux.client = Client API
>  org.cometd.bayeux.server = Server API
>
>  org.cometd.server = server impl
>  org.cometd.client = client impl

Agreed.

> For naming within the client, I think we need to be careful with
> the use of the work "client" for any entities.  This will
> be good for getting away from the confusion of server-side
> clients vs client side clients.
>
> I think the top level interfaces for the key concepts
> should just be for identity and not for usage:

If they're "not for usage", they have no reason to exist for me.
Also, would make little sense to have them as a common ancestor, as
you never see/use the common ancestor anyway...

> Then the client API can be:
>
>  package org.cometd.bayeux.client;
>
>    interface Bayeux
>    {
>      Session handshake(...);
>    }
>
>    interface Session extends Client
>    {
>       ClientChannel getClientChannel(String channel);
>       void disconnect();
>    }

Ok.

>    interface ClientChannel extends Client,Channel
>    {
>      void publish(Object msg);
>      Subscription subscribe(MessageListener listener);
>    }

ClientChannel "is-a" Client ? Does not sound right.
I'll work on this and see if it's really needed.

> And on the server we can have
>
>  package org.cometd.bayeux.server;
>
>    interface Bayeux
>    {
>      LocalSession newLocalSession(String idBase);
>      addListener(BayeListener listener);
>      ServerChannel getChannel(String channelId);
>    }
>
>    public interface ClientBayeuxListener extends BayeuxListener
>    {
>      public void clientAdded(Session client);
>      public void clientRemoved(Session client);
>    }
>
>    interface Session extends Client
>    {
>      void deliver(msg);
>      addMessageListener(MessageListener listener);
>      void disconnect();
>    }

As above, the mere fact that Session as getClientId() does not
justify, for me, to inherit from Client.

>    interface LocalSession extends Session, org.cometd.bayeux.client.Session
>    {
>    }

This also sound strange... (as in "a server side session "is-a" client
side session" ?!?)
I would frankly rename server.Session to ServerSession maybe, and
again I think that the fact they have a similar API does not justify
inheritance.
You can make that dependency in the implementation if you need to
reuse code (e.g. ServerSessionImpl extends ClientSessionImpl
implements ServerSession), but I doubt anyone in the server would want
to see a client.Session ;)

>    interface ServerChannel extends Channel
>    {
>      boolean isLazy();
>      boolean isPersistent();
>      boolean isBroadcast();  // !meta and !service
>
>      Set<Session> getSubscribers();

Mmm, nope. I would call the ServerChannel with some other method and
not expose its subscribers, if possible.
This allows random Joe to do:

ServerChannel c = ...
c.getSubscribers().clear();

which makes no sense even if we return a snapshot of the subscribers.
I'm for not providing methods if they make no sense and expose internals.

>      void addDataFilter(DataFilter filter);
>      void addListener(ChannelListener listener);
>    }

Simon
--
http://bordet.blogspot.com
---
Finally, no matter how good the architecture and design are,
to deliver bug-free software with optimal performance and reliability,
the implementation technique must be flawless. Victoria Livschitz

[Greg Thomas]

2009/12/9 Greg Wilkins <gr...@intalio.com>

All,

Simone and I have started working on cometd 2.0, which is a refactor of the
code base of cometd1.0 with the two purposes of:

 + cleaning up dead end ideas that hang over from the development of
  cometd and bayeux
 + to support websocket and other new transports.

Can I take this opportunity to say thanks for the efffort you've put in for the 1.0 release; it's a really nice framework you've got in place, and I'm sure that very many people will be able to take advantage of it.

Also, can I make a suggestion for 2.0? I'm mainly dealing with the JavaScript client, and I think I can see something that may make a useful addition here. Currently, when writing a client, it's necessary to subscribe to various meta/* channels to determine when the client has connected to the server (so that you can send a subscribe request), when it's disconnected, etc. etc.

e.g. http://svn.cometd.org/trunk/cometd-javascript/examples-jquery/src/main/webapp/jquery-examples/chat/chat.js has functions that are called from the _metaConnect handler - connectionEstablished, Broken, Closed.

I think it would make sense to include the functionality within the example _metaConnect handler in the core API; after all I suspect the majority of the users of the API will be implementing something very like that, so why not include it in the API?

Anyway, thanks.again for the work; I'll try to stop pestering the list with awkward questions ;)

Greg

[Greg Wilkins]

Simone Bordet wrote:

>>
>> package org.cometd.bayeux.client;
>>
>> interface Bayeux
>> {
>> Session handshake(...);
>> }
>>
>> interface Session extends Client
>> {
>> ClientChannel getClientChannel(String channel);
>> void disconnect();
>> }
>
> Ok.
>
>> interface ClientChannel extends Client,Channel
>> {
>> void publish(Object msg);
>> Subscription subscribe(MessageListener listener);
>> }
>
> ClientChannel "is-a" Client ? Does not sound right.
> I'll work on this and see if it's really needed.

I've renamed this ChannelSession.

I agree that it is not a *is-a* relationship with Client
and should probably not extend Client.

However, I do think the top level Client interface is
good to group together all the different representations
we have of client:

client side session is a client representation
server side session is a client representation
server side local session is a client representation

So I think I went a bit far with the use of Client and
will just make those three extend it.

>> And on the server we can have
>>
>> package org.cometd.bayeux.server;
>>
>> interface Bayeux
>> {
>> LocalSession newLocalSession(String idBase);
>> addListener(BayeListener listener);
>> ServerChannel getChannel(String channelId);
>> }
>>
>> public interface ClientBayeuxListener extends BayeuxListener
>> {
>> public void clientAdded(Session client);
>> public void clientRemoved(Session client);
>> }
>>
>> interface Session extends Client
>> {
>> void deliver(msg);
>> addMessageListener(MessageListener listener);
>> void disconnect();
>> }
>
> As above, the mere fact that Session as getClientId() does not
> justify, for me, to inherit from Client.

true

>> interface LocalSession extends Session, org.cometd.bayeux.client.Session
>> {
>> }
>
> This also sound strange... (as in "a server side session "is-a" client
> side session" ?!?)

with the latest naming, this becomes:

LocalSession extends ServerSession, ClientSession

which says to me, this is an object that has both the client side
and the server side APIs - which is true. It can subscribe and
publish like a remote client, but it can also be delivered to
like a server side client handle.

> I would frankly rename server.Session to ServerSession maybe, and
> again I think that the fact they have a similar API does not justify
> inheritance.

Done.

>
>> interface ServerChannel extends Channel
>> {
>> boolean isLazy();
>> boolean isPersistent();
>> boolean isBroadcast(); // !meta and !service
>>
>> Set<Session> getSubscribers();
>
> Mmm, nope. I would call the ServerChannel with some other method and
> not expose its subscribers, if possible.

It is not exposed to subscribers. Subscribers go via the
ClientSession to get a ChannelClient to call subscribe.

ServerChannels are for the implementers of server side services.

> This allows random Joe to do:
>
> ServerChannel c = ...
> c.getSubscribers().clear();
>
> which makes no sense even if we return a snapshot of the subscribers.
> I'm for not providing methods if they make no sense and expose internals.

We can either make this an immutable set or we can make clear implement
a mass unsubscribe.

I much rather expose a suitably protocted collection that have

int getNumberOfSubscribers()
Interator<Session> getSubscriberIterator()

I think most of the more recent java APIs have used exposed
collections in this way - but not hugely important either way.

cheers

[Simone Bordet]

Hi,

On Wed, Dec 9, 2009 at 22:59, Greg Wilkins <gr...@intalio.com> wrote:
>>>    interface ServerChannel extends Channel
>>>    {
>>>      boolean isLazy();
>>>      boolean isPersistent();
>>>      boolean isBroadcast();  // !meta and !service
>>>
>>>      Set<Session> getSubscribers();
>>
>> Mmm, nope. I would call the ServerChannel with some other method and
>> not expose its subscribers, if possible.
>
> It is not exposed to subscribers.    Subscribers go via the
> ClientSession to get a ChannelClient to call subscribe.

? It exposes a Set<Session>, which are the subscribers, no ?

> ServerChannels are for the implementers of server side services.

And why I would want to expose the subscribers in that way ?

>> This allows random Joe to do:
>>
>> ServerChannel c = ...
>> c.getSubscribers().clear();
>>
>> which makes no sense even if we return a snapshot of the subscribers.
>> I'm for not providing methods if they make no sense and expose internals.
>
> We can either make this an immutable set or we can make clear implement
> a mass unsubscribe.

Sure but I disagree on the way it's done.
I much prefer ServerChannel.unsubscribeAll(), if there is such a use
case (which I doubt).

> I much rather expose a suitably protocted collection that have
>
>   int getNumberOfSubscribers()
>   Interator<Session> getSubscriberIterator()
>
> I think most of the more recent java APIs have used exposed
> collections in this way - but not hugely important either way.

Quite horrible :)
And let's not take the java collections as an example of good design,
they are used in OO courses as example of how NOT do things :)
This approach has several drawbacks (concurrency is broken or too
implicit, etc, etc).

Why you need the number of subscribers, or iterating over the
subscribers with an external iterator ?

The smallest the API the best.

[Simone Bordet]

Hi,

On Wed, Dec 9, 2009 at 23:29, Simone Bordet <simone...@gmail.com> wrote:
> The smallest the API the best.

To reiterate the concept, I think BayeuxServer must not have methods
such as newLocalSession() or getChannels(), getSessions() nor
publish().
These are all implementation details, and BayeuxServer is part of the
API exposed to users.
These methods can be put on an internal interface, iff they're needed.
Methods publish() and publishLazy() should be on a Channel not on
BayeuxServer (I guess it's a typo).

My ideal BayeuxServer has:

setBayeuxPolicy(BayeuxPolicy policy);
Registration registerService(BayeuxService service);
Registration registerExtension(Extension extension);

There is no need to get the BayeuxPolicy (after all, it's the user
that sets it, so it has it already, but I can accept a getter for it),
and I would like an explicit API for services (to avoid a "new
EchoService(bayeux)" which raises the question: "it's GC'ed the line
after... why it works ?").

I was also thinking that the Session-scoped channels may not be a good
idea after all... though the API is really simple. I think they
complicate the implementation quite a bit, but will have a better
feeling about by tomorrow.
Something like:

Channels.from("/foo").publish(session, data);

much preferred over:

session.publish(Channels.from("/foo"), data);

Cheers,

[Greg Wilkins]

Simone Bordet wrote:
> Hi,
>
> On Wed, Dec 9, 2009 at 23:29, Simone Bordet <simone...@gmail.com> wrote:
>> The smallest the API the best.
>
> To reiterate the concept, I think BayeuxServer must not have methods
> such as newLocalSession() or getChannels(), getSessions() nor
> publish().

Simone,

I think I can justify some of these:


BayeuxServer.newLocalSession(...) is analogous to BayeuxClient.handshake(...)

You create a new session by going to the Bayeux instance (client or server)
and asking for a session, passing the needed args.


The publish methods are needed for services/extension like Oort that receive
messages and republish them. They already have a message instance and they
might be publishing on behalf of a different client, so they need a way
to put an existing message back into the mix.

I think getChannels(), getSessions() are good things to have
on a server as management interface.... but I guess they
could be on the impls rather than the main public interface.
I'll take them off for now... but I bet there will be a need for them.

> These are all implementation details, and BayeuxServer is part of the
> API exposed to users.

remember that users on the server include:

server side clients (LocalSession)
server side services
extensions

So there needs to be some wider APIs to support all
there needs.

> These methods can be put on an internal interface, iff they're needed.
> Methods publish() and publishLazy() should be on a Channel not on
> BayeuxServer (I guess it's a typo).

No not a typo. It is how you publish an already
fully formed message. Remember that a message already
has a channel, so ServerChannel.publish(Message m)
would allow channel to be specified twice and thus
can result in conflicts.

> My ideal BayeuxServer has:
>
> setBayeuxPolicy(BayeuxPolicy policy);
> Registration registerService(BayeuxService service);
> Registration registerExtension(Extension extension);
>
> There is no need to get the BayeuxPolicy (after all, it's the user
> that sets it, so it has it already, but I can accept a getter for it),
> and I would like an explicit API for services (to avoid a "new
> EchoService(bayeux)" which raises the question: "it's GC'ed the line
> after... why it works ?").

BayeuxService was always very much an addon because there was
no ServerSession class. With a ServerSession (and a proper
subscribe method), the need for BayeuxService is reduced...
although the reflection aspects might still be good.

I'm not sure what registering a service would mean?

> I was also thinking that the Session-scoped channels may not be a good
> idea after all... though the API is really simple. I think they
> complicate the implementation quite a bit, but will have a better
> feeling about by tomorrow.
> Something like:
>
> Channels.from("/foo").publish(session, data);
>
> much preferred over:
>
> session.publish(Channels.from("/foo"), data);

The usage as it is now is

session.getChannelSession("/foo/bar").publish(data);

which I guess could be reduced to:

session.channel("/foo/bar").publish(data);


I don't think the implementation will be that difficult, as
it will just be something like


ChannelClientImpl
{
ChannelImpl channel;
ClientImpl from

public void publish(Object data)
{
channel.publish(from,data);
}
}



Anyway, I've committed some simplifications and other stuff
from your feedback.

cheers

[Greg Wilkins]

Simone,

I tried to bring in MetaChannels to the API again.

I still don't like it so much, because it is a lot of
extra code (4 interfaces and implementations) just
to implement a simple restriction that clients should not
pub/sub to meta channels. A simple IllegalArgumentException
achieves the same with much less code.

cheers

[Greg Wilkins]

I'm still playing around with the API for 2.0

https://svn.cometd.org/branches/cometd2/cometd-java/bayeux-api

I've moved it back into a client/server package hierarchy
as it was getting too fat for a single package.


I've think that Message must extend Map<String,Object>
because there are just too many optional fields that would
need to be put into the API (eg connectionType, timestamp,
version, error, successful, etc.)

I think being a map future proofs the impl a bit better anyway.

I have however, created a Message.Mutable subtype, that will
indicate when a message is actually able to be changed (eg
in an extension), and when it can't be (in a Listener).

I've added all the Listeners that we currently have, but
simplified the marker interface mechanism.



I'm still not 100% on the

Subscription s=ClientSession.getChannel("/foo/bar").subscribe(listener);

Style, because it needs the intermediary ChannelSession
class. But other choices for API end up in the eternal
debate of is it channel.subscribe(client,listener) or
client.subscribe(channel,listener) or both.

So currently I'm sticking with ChannelSession (although
a better name might help me like it more).


Another issue is the ability to access the current HTTP request.
Currently this is on the server BayeuxImpl, but really should
be exposed in the API. But I don't want the API to depend
on the servlet API, plus we need to be able to handle websocket
that does not have a HTTP request (other than the initial update).

cheers

[Simone Bordet]

Hi,

On Mon, Dec 14, 2009 at 02:06, Greg Wilkins <gr...@intalio.com> wrote:
> I've think that Message must extend Map<String,Object>
> because there are just too many optional fields that would
> need to be put into the API (eg connectionType, timestamp,
> version, error, successful, etc.)
>
> I think being a map future proofs the impl a bit better anyway.
>
> I have however, created a Message.Mutable subtype, that will
> indicate when a message is actually able to be changed (eg
> in an extension), and when it can't be (in a Listener).

Mmm, is it really needed ?
I can always call Message.remove("clientId"), so what would be Mutable
for (as it is already mutable) ?

I still think that the API does not need Message to be a Map; the
implementation may need this, but we'll see.

> I've added all the Listeners that we currently have, but
> simplified the marker interface mechanism.
>
> I'm still not 100% on the
>
>  Subscription s=ClientSession.getChannel("/foo/bar").subscribe(listener);
>
> Style, because it needs the intermediary ChannelSession
> class.   But other choices for API end up in the eternal
> debate of is it channel.subscribe(client,listener) or
> client.subscribe(channel,listener) or both.
>
> So currently I'm sticking with ChannelSession (although
> a better name might help me like it more).
>
>
> Another issue is the ability to access the current HTTP request.
> Currently this is on the server BayeuxImpl, but really should
> be exposed in the API.   But I don't want the API to depend
> on the servlet API, plus we need to be able to handle websocket
> that does not have a HTTP request (other than the initial update).

Should not it be in the http transport ?

[Greg Wilkins]

Simone Bordet wrote:
> Hi,
>
> On Mon, Dec 14, 2009 at 02:06, Greg Wilkins <gr...@intalio.com> wrote:
>> I've think that Message must extend Map<String,Object>
>> because there are just too many optional fields that would
>> need to be put into the API (eg connectionType, timestamp,
>> version, error, successful, etc.)
>>
>> I think being a map future proofs the impl a bit better anyway.
>>
>> I have however, created a Message.Mutable subtype, that will
>> indicate when a message is actually able to be changed (eg
>> in an extension), and when it can't be (in a Listener).
>
> Mmm, is it really needed ?
> I can always call Message.remove("clientId"), so what would be Mutable
> for (as it is already mutable) ?

Simone,

Maps can be immutable and I think the maps passed to normal listeners
should be.

Passing a mutable message is a key indicator that a message may
be modified without breaking the implementation.

> I still think that the API does not need Message to be a Map; the
> implementation may need this, but we'll see.

I think the number of optional fields in the top level makes
a Map make sense, as well as having a common mapping the json.

>> Another issue is the ability to access the current HTTP request.
>> Currently this is on the server BayeuxImpl, but really should
>> be exposed in the API. But I don't want the API to depend
>> on the servlet API, plus we need to be able to handle websocket
>> that does not have a HTTP request (other than the initial update).
>
> Should not it be in the http transport ?

It definitely should be in the HTTP transport. But I was thinking
that Transports would be an implementation specific thing. If we
want portable extensions, then that means that HttpTransport needs
to be an API interface... at least and abstract version of it.


cheers

[Greg Wilkins]

Simone,

After 2 days work on my exceedingly clever ImmutableHashMap implementation....
I'm unfortunately going to have to concede that it is clever AND SLOW!

The map itself is much the same as a hash map. A bit slower to
create, but then a bit faster to access common fields.

But in order for it to have any real benefit, it needs to be pooled, and
as if I simulate a message pool in the benchmark, it is a clear loser!
About 30% slower!!!!

So modern JVMs are awesome with their GC!

Thus I think the clever impl will need to be put aside and
we can just go with simple hashmaps allocated as needed.
The down side, is that we cannot have deep immutability within
a message, but we don't have that now, so no loss.


cheers

[Greg Wilkins]

So I think I've just proved that I'm and idiot, which in turn proves
that I'm not an idiot!

I think my ImmutableHashMap implementation was not correctly recycling
the ImmutableHashMaps. I've now written both an ImmutableMessage
and a HashMapMessage implementation and the benchmark now shows that
the pooled ImmutableMessage is about 35% faster than just using HashMap
plus it provides an enforced immutable interface!

So I don't think all my object creation phobia is that outdated after all!

Simone, can you double check this benchmark (checked in as MessageBenchmark)

thanks

[Peter Saitz]

On Sun, Dec 20, 2009 at 10:06 PM, Greg Wilkins <gr...@intalio.com> wrote:

 So I think I've just proved that I'm and idiot, which in turn proves
that I'm not an idiot!

 

Quote of the century! :-)

[Simone Bordet]

Hi,

On Mon, Dec 21, 2009 at 07:06, Greg Wilkins <gr...@intalio.com> wrote:
>
>
> So I think I've just proved that I'm and idiot, which in turn proves
> that I'm not an idiot!
>
> I think my ImmutableHashMap implementation was not correctly recycling
> the ImmutableHashMaps.    I've now written both an ImmutableMessage
> and a HashMapMessage implementation and the benchmark now shows that
> the pooled ImmutableMessage is about 35% faster than just using HashMap
> plus it provides an enforced immutable interface!
>
> So I don't think all my object creation phobia is that outdated after all!
>
> Simone, can you double check this benchmark (checked in as MessageBenchmark)

Erhm...

In that test class I see that you use HashMaps in
immutableMessageTest(), and pooled ImmutableMessages in
hashMessageTest() - should be the opposite, no ?
I see a 40% gain, but following the code it's the HashMap that gains
on the ImmutableMessage...

Can you triple check ?