optionally skip node creation

[fippo]

had to make it work with already created nodes.

[emcho]

Why does this need to be a configuration option? Wouldn't it be better to simply check if the node is there and automatically skip creation if it isn't?

[fippo]

well, you could test by creating a node... but that still gives way to many ways to shoot yourself in the foot. FWIW, sane xmpp servers have autocreate options :-)

[emcho]

Is this the only way to check? How about subscribing? Also why would a tentative create mean you are shooting yourself in the foot? Could you please be more specific?

[emcho]

oh and by the way, please make sure you are not going beyond column 80.
https://jitsi.org/codeconvention

[fippo]

my node is configured already, so trying to create it will fail (with a forbidden error I think -- conflict is handled but prosody returns a different error so the current strategy is not sufficient).

will fix the 80.

[emcho]

So you are basically saying that there is no reliable standard way in XMPP to query for an existing pubsub node?

[emcho]

Like this for instance:

http://xmpp.org/extensions/xep-0060.html#entity-nodes

[fippo]

Sure, that's just way more complicated. Unless you want to have a flow like create+fail+publish-anyway+get-rejected.

Assuming you have to create a node before being able to publish to it is not covered by xep-0060 afaics.

[emcho]

See my previous comment. Why would disco not work?

Also, why would you need a flow like this "create+fail+publish-anyway+get-rejected". Why would you get rejected?

[fippo]

(1) How many hierarchies of node collections do you want to support? Also, I would note that currently there is no service discovery as described in http://xmpp.org/extensions/xep-0060.html#entity-features to figure out whether the jid published to is a pubsub service.

(2) because the current assumption that you can publish when you fail with a conflict error is wrong / not covered by the spec.

[emcho]

(1) How many hierarchies of node collections do you want to support?

As many as there are in the configuration option.

Also, I would note that currently
there is no service discovery as described in http://xmpp.org/extensions/xep-0060.html#entity-
features to figure out whether the jid published to is a pubsub service.

You mean prosody does not support this?

(2) because the current assumption that you can publish when you fail with a conflict error
is wrong / not covered by the spec.

The assumption is that if you are configured with a specific room then you can either create it or it would turn out that it exists in which case you should be able to publish. If that is impossible, then the deployer has an issue and they need to be notified about it.

I am probably missing something but I don't see how this patch has anything to do with this case. It basically spares you the need to attempt creation and the only benefit of that is a single RTT for the entire lifecycle of a bridge instance. One would still need to handle the case where a messages can be rejected. The deployer would still have to be notified about a misconfiguration.

[fippo]

(2) I don't know. Nor do I care. I'd note that (5.1) discovery is currently not done by the JVB pubsub publisher. Given that it is a single RTT for the entire lifetime I presume that's a bug?
Once that is done it would probably be good to also have the full process of discovering whether the publisher is an owner as described in section 5.4. I'd note that, like node creation, this is not a required precondition.

(3) your assumption seems to be that being able to create a node is a precondition to publishing. This is wrong. My node owners JID is different from the JVB, however I have allowed the JVB to publish to that node by granting it publisher rights. The JVB might NOT be allowed to create its own nodes on my pubsub server for security reasons (such as restricting creation to local jids or admins).
Did you notice that this doesn't change the current default behaviour but makes an override configurable?

[emcho]

(2) I don't know. Nor do I care.

Well ... I do.

I'd note that (5.1) discovery is
currently not done by the JVB pubsub publisher. Given that it is a
single RTT for the entire lifetime I presume that's a bug? Once that
is done it would probably be good to also have the full process of
discovering whether the publisher is an owner as described in section
5.4. I'd note that, like node creation, this is not a required
precondition.

Sounds good.

(3) your assumption seems to be that being able to create a node is a
precondition to publishing.

Not at all. My assumption is that trying to create the node can do no hard but still do good.

This is wrong.

Yes, of course.

My node owners JID is
different from the JVB, however I have allowed the JVB to publish to
that node by granting it publisher rights. The JVB might NOT be
allowed to create its own nodes on my pubsub server for security
reasons (such as restricting creation to local jids or admins).

Agreed!

Did you notice that this doesn't change the current default behaviour but
makes an override configurable?

Yes, sure. The point is that if something doesn't absolutely require a configuration option, there shouldn't be one. I am asking my question again: why is it a problem for you if the bridge does a failed attempt to create a room every time it starts? Are you trying to save the extra RTT during startup?

[fippo]

because my pubsub service doesn't allow node creation by non-local entities (well, it might). Also, I am letting multiple bridges publish their stats to the same pubsub node that acts as an aggregator. How many more usecases do you need?

why is it a problem for you if the bridge does a failed attempt to create a room every time it starts?

Because the bridge's currently is to give up when it can't create the node. We can agree this is a bug because of that invalid assumption that node-creation is a precondition to publish.

[emcho]

because my pubsub service doesn't allow node creation by non-local
entities (well, it might).

Not a problem!

Also, I am letting multiple bridges
publish their stats to the same pubsub node that acts as an
aggregator.

Not a problem either!

How many more usecases do you need?

One?

why is it a problem for you if the bridge does a failed attempt to
create a room every time it starts?
Because the bridge's currently is
to give up when it can't create the node.

And that's something I can certainly agree would need fixing.

[fippo]

"Not a problem!"
-- well, it won't reply with a conflict error in that case, so https://github.com/jitsi/jitsi-videobridge/blob/master/src/org/jitsi/videobridge/pubsub/PubSubPublisher.java#L322 wont work.

"Not a problem either!"
No conflict error either.

"And that's something I can certainly agree would need fixing."
The thing that needs to be fixed is the assumption that publication always needs creation.
Some smart servers will actually create a node if you publish to it. Not all of them though I think. So having a way to avoid creation is good while keeping an easy-to-use default that works for most simple usecases.

[emcho]

"Not a problem!"
-- well, it won't reply with a conflict error in that case, so
https://github.com/jitsi/jitsi-videobridge/blob/master/src/org/jitsi/videobridge/pubsub/PubSubPublisher.java#L322
wont work.

Indeed. Not with the current implementation.

"And that's something I can certainly agree would need fixing."
The
thing that needs to be fixed is the assumption that publication needs
creation. Some smart servers will actually create a node if you
publish to it. Not all of them though I think.

I think you are trying to say that we should handle the case where servers return other errors than "CONFLICT" in case of an existing room and I already agreed this needs to be addressed.

In other words, to make it clearer, I am not disagreeing there's a problem to be fixed here. I just don't think it should be addressed with a config option.