Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'video chats served' counter to the server #38

Merged
merged 2 commits into from
Apr 10, 2018
Merged

Conversation

karth295
Copy link
Contributor

@karth295 karth295 commented Apr 7, 2018

WIP: unit tests

@toomim please help me make these tests work. I think the tests are failing because state updates are async and I should only check that state has changed after a timer. Or is there a better strategy?

Linking to:

@karth295 karth295 requested a review from toomim April 7, 2018 19:25
@karth295 karth295 merged commit e2951da into master Apr 10, 2018
@karth295 karth295 deleted the chats-served branch April 10, 2018 06:52
bus.state.chats_served += diff.size;

previous_spaces = new_spaces;
})();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey I should have mentioned this before. The purpose of this reactive function is to define the state chats_served, and you can define that directly with a .to_fetch function:

bus('chats_served').to_fetch = (old) => {
  const new_spaces = get_spaces(bus.fetch('connections'))
  const diff = set_difference(previous_spaces, new_spaces)
  previous_spaces = new_spaces
  return {_: old._ + diff}
}

I think we need a better name than .to_fetch... perhaps .define.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@toomim thanks -- I didn't realize to_fetch gave you the old value -- I thought it was the key that gets fetched.

Question: is there a way to hook into the to_save function of /connection from a statebus server?. I realize this code would be far more efficient if we avoided recomputing these sets every single time a connection is saved. Aka every time somebody moves their cursor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karth295, the to_fetch function looks at its parameter name to figure out what to give it. You can do any of these:

bus('chats_served').to_fetch = (old) => {
bus('chats_served').to_fetch = (key) => {
bus('chats_served').to_fetch = (k, old) => {
bus('chats_served').to_fetch = (key, old) => {
bus('chats_served').to_fetch = (old, k) => {

Copy link
Member

@toomim toomim Apr 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karth295 you can define a master('connections').to_save = (obj) => {...} function, sure. There is no such function now. How are you thinking of making it more efficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, every time a connection is updated in any form, we need to do an O(connections) operation to determine whether to increment this chats_served counter. I think it could be O(1) if there were a way to get a callback for a specific connection changing. Like master('connection/*').to_save).

Alternatively, is this possible through the client-specific bus?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. We don't yet have a connection/* state for each connection, but it's in the plans: https://wiki.invisible.college/statebus/dev

Also, we'll eventually be adding handlers for specific types of diffs, so that you can get a callback just when the list of connections gets smaller or larger.

In the meantime, we can use this template:

master = require('statebus').serve({
  client: (client, connection) => {
    # Handle a new connection here
    console.log('Connection open!')
    connection.on('close', _=> console.log('Connection closed!'))
  }
})

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we'll eventually be adding handlers for specific types of diffs, so that you can get a callback just when the list of connections gets smaller or larger.

Actually, we've already got enough diff support in statebus that we could implement this now. We'd modify the implementation of the connections state in server.js so that each time it adds or removes a connection, it also communicates the patch. For instance, we'd change this code in server.js:

                // Close connection
                if (user_bus_func) {
                    delete connections[conn.id]; master.save(connections)
                    user.delete_bus()
                }

to this:

                // Close connection
                if (user_bus_func) {
                    delete connections[conn.id]
                    master.save('connections', {patch: `delete .${conn.id}`})
                    user.delete_bus()
                }

And we'd replace this:

                // Open connection
                connections[conn.id] = {client: conn.id}; master.save(connections)

with this:

                // Open connection
                connections[conn.id] = {client: conn.id}
                master.save('connections', {patch: `.${conn.id} = {"client": "${conn.id}"}`})

Then we can look for the patch in a master('connections').on_save = (o, t) => { if (t.patch) ... } handler.

Now, this probably isn't worth doing right now. But I just wanted to take the opportunity to explore some of the future power we're developing with the diff/sync/patch architecture.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and augmenting saves with these patches might not be worth doing ever — because they will be auto-generated in the future, once we get the new JSON encoding, and revamp the state proxy object to use it, we can have the state object auto-generate a patch for each change you make.

So we can just wait until that's done, and then we'll be able to get O(1) updates without much additional work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants