Opened 3 years ago

Closed 16 months ago

Last modified 15 months ago

#3022 closed defect (fixed)

[PATCH] Split rating from lobby bot into another

Reported by: leper Owned by: scythetwirler
Priority: Should Have Milestone: Alpha 21
Component: Multiplayer lobby Keywords: patch
Cc: scythetwirler Patch:

Description

The rating functionality should be split from the bot into another, so that enabling/disabling/changing the rating system can be done independently of the main bot match making functionality.

(Also the rating system should be profiled and improved as it is slow with user numbers > 40)

Attachments (3)

mod_gamelist.erl (5.0 KB) - added by Josh 23 months ago.
WIP version of the new module
LobbyBotSplit.patch (61.3 KB) - added by scythetwirler 20 months ago.
LobbyBotSplitv2.patch (61.5 KB) - added by scythetwirler 20 months ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 23 months ago by Josh

I've been making good progress on this. To help minimize our dependencies and reduce the required configuration, I've wrapped up all the gamelist management into a new ejabberd module which replaces the old one. It dynamically intercepts gamelist stanzas from any channel and will eventually spawn a new handler for each channel (at the moment, all the channels are handled by one instance). I'm considering removing the changestate message type, as it's job could be done by a register message.

The current todo list is:

  • Make this work per-room
  • Determine what to do about changestate
  • Multicast updates

Changed 23 months ago by Josh

Attachment: mod_gamelist.erl added

WIP version of the new module

comment:2 Changed 23 months ago by leper

Locking ourself to a specific XMPP server is a bad idea. A nicer solution would be to use the Publisher-Subscriber XEP-60. But none of this is in the scope of this ticket which is simply about having two different bots running, which isn't a huge configuration overhead since nearly all of that is automated already.

comment:3 in reply to:  2 ; Changed 23 months ago by Josh

Replying to leper:

Locking ourself to a specific XMPP server is a bad idea. A nicer solution would be to use the Publisher-Subscriber XEP-60. But none of this is in the scope of this ticket which is simply about having two different bots running, which isn't a huge configuration overhead since nearly all of that is automated already.

I'm having difficultly seeing why developing for a specifically for ejabberd is an issue. Our system already would be entirely broken without the ejabberd module and ejabberd supports every major platform. Also, I see no signs of ejabberd going away. It's the most widely used XMPP server in the world and is very actively developed.

In the case that someone wants to use a different server, they could use the old Python bot or use ejabberd as a remote service provider. The bot is relatively simple, so it could even be re-written from scratch for a different server.

In my opinion, this proposed approach is more scalable and moves towards getting rid of the SleekXMPP dependency (as SleekXMPP is no longer actively developed and behaves poorly with SQLAlchemy). I will look into using XEP-60 with this approach, as that sounds like a nice solution.

comment:4 in reply to:  3 Changed 23 months ago by leper

Replying to Josh:

I'm having difficultly seeing why developing for a specifically for ejabberd is an issue. Our system already would be entirely broken without the ejabberd module and ejabberd supports every major platform. Also, I see no signs of ejabberd going away. It's the most widely used XMPP server in the world and is very actively developed.

Lock in is always an issue, whether it is one now is not the point. It might end up being an issue in the future and the current module is quite minimal in what it needs to do, so the needed porting work when switching is kept to a minimum.

In the case that someone wants to use a different server, they could use the old Python bot or use ejabberd as a remote service provider. The bot is relatively simple, so it could even be re-written from scratch for a different server.

Which will not be maintained anymore, and someone will change the protocol so that this is not a solution. Having to rewrite things just because we don't think about maintainability when making changes seems like a bad trade-off.

In my opinion, this proposed approach is more scalable and moves towards getting rid of the SleekXMPP dependency (as SleekXMPP is no longer actively developed and behaves poorly with SQLAlchemy). I will look into using XEP-60 with this approach, as that sounds like a nice solution.

Then we should switch to something that is maintained. (And also switch away from using an ORM mapper when we do not need one since that is a solution in search of a problem in the bot. It likely isn't any more scalable since we'd need to run that module on each ejabberd instance (which is how one scales XMPP) and such would fragment the gamelist. As far as I can see using pubsub would alleviate this scaling issue, but as said that is not the point of this ticket, which is making sure that we can iterate the rating bot independently from the main functionality of the lobby.

comment:5 Changed 20 months ago by scythetwirler

Milestone: BacklogAlpha 20
Owner: changed from Josh to scythetwirler
Status: newassigned

Changed 20 months ago by scythetwirler

Attachment: LobbyBotSplit.patch added

comment:6 Changed 20 months ago by scythetwirler

Keywords: patch review added
Summary: Split rating from lobby bot into another[PATCH] Split rating from lobby bot into another

Changed 20 months ago by scythetwirler

Attachment: LobbyBotSplitv2.patch added

comment:7 Changed 20 months ago by scythetwirler

Milestone: Alpha 20Alpha 21

Can be committed anytime, even between releases.

comment:8 in reply to:  7 Changed 17 months ago by stanislas69

Keywords: rfc added; review removed

From 2016-05-15:

23:56 <@leper> Can you commit the rating bot splitup?
23:56 <@leper> (no isn't a valid answer)
23:57 < scythetwirler> leper: even with the remaining issue?
23:58 <@leper> which one?
23:58 < scythetwirler> the crash?
23:58 <@leper> hm
23:58 <@leper> has anyone checked what could cause it?
23:58 <@leper> (but the answer probably still is a yes)
23:59 < elexis> if its the sleekxmpp update, cant you test the code with an older version?
23:59 < scythetwirler> attempted yes, identified, no
23:59 < scythetwirler> it is an old version :P
00:00 <@leper> tried with a new one?
00:01 < scythetwirler> new one would require some code changes

Moving it to the rfc queue

Last edited 17 months ago by elexis (previous) (diff)

comment:9 Changed 16 months ago by scythetwirler

Resolution: fixed
Status: assignedclosed

In 18609:

Splits lobby bot into a ratings bot and a main bot. Fixes #3022.

comment:10 Changed 15 months ago by fatherbushido

Keywords: rfc removed

comment:11 in reply to:  9 ; Changed 15 months ago by Josh

Replying to scythetwirler:

In 18609:

Splits lobby bot into a ratings bot and a main bot. Fixes #3022.

Thanks for the patch! I only see the reorganization of the main bot here. Could you please point me to the new ratings bot?

I am still working on some alternative implementations, but this should make my work easier. Ideally we can implement a basic XMPP pub/sub service for the gamelist, and have separate high performance code for ratings.

comment:12 Changed 15 months ago by elexis

See r18610

comment:13 in reply to:  11 Changed 15 months ago by scythetwirler

Replying to Josh:

Replying to scythetwirler:

In 18609:

Splits lobby bot into a ratings bot and a main bot. Fixes #3022.

Thanks for the patch! I only see the reorganization of the main bot here. Could you please point me to the new ratings bot?

I am still working on some alternative implementations, but this should make my work easier. Ideally we can implement a basic XMPP pub/sub service for the gamelist, and have separate high performance code for ratings.

See #4203.

Note: See TracTickets for help on using tickets.