Vanchi Koduvayur vanchi-zendesk

  • Zendesk
  • San Francisco
  • Joined on
vanchi-zendesk commented on pull request zendesk/radar#131
@vanchi-zendesk

sorry that comment went on the commit. Please use var here.

vanchi-zendesk commented on pull request zendesk/radar#131
@vanchi-zendesk

:+1: (after we use var the two variables.)

@vanchi-zendesk

use var

vanchi-zendesk commented on pull request zendesk/radar#131
@vanchi-zendesk

This should be set once when the server starts.

vanchi-zendesk commented on pull request zendesk/radar#131
@vanchi-zendesk

maybe !client.update(message) works better?

vanchi-zendesk commented on pull request zendesk/persistence#11
@vanchi-zendesk

:+1:

vanchi-zendesk commented on pull request zendesk/radar_client#48
@vanchi-zendesk

:+1: I am happy that this crazy 'prop' setting code will go away when we have radar_protocol package.

vanchi-zendesk commented on pull request zendesk/maxwell#48
@vanchi-zendesk

:+1:

vanchi-zendesk commented on pull request zendesk/persistence#11
@vanchi-zendesk

To be clear: The callback for get, will be called after we exec. So, this expire will never be called.

vanchi-zendesk commented on pull request zendesk/persistence#11
@vanchi-zendesk

These tests should also assert that there is a ttl set. Persistence.redis().ttl(key, callback) should get you that. It should have a ttl between 0 …

vanchi-zendesk commented on pull request zendesk/persistence#11
@vanchi-zendesk

This should be done outside the callback. Multi works like a queue of commands. So, add a get, then expire and then exec. If you expire a key which…

vanchi-zendesk commented on pull request zendesk/radar#131
@vanchi-zendesk

sure, that makes sense.

vanchi-zendesk commented on pull request zendesk/radar#131
@vanchi-zendesk

We need tests with the name_id_sync message from the client to see if that works.

vanchi-zendesk commented on pull request zendesk/radar#131
@vanchi-zendesk

Not sure why this change is made. Existing tests should not break.

vanchi-zendesk commented on pull request zendesk/radar#131
@vanchi-zendesk

suggestion: rename rtn -> is_authorized

vanchi-zendesk commented on pull request zendesk/radar#131
@vanchi-zendesk

Here, instead of passing the id, it's better to pass the Client (the new one) object down, so we can just do client.dataStore(). Also, this is why …

vanchi-zendesk commented on pull request zendesk/radar#131
@vanchi-zendesk

we probably need not store clientVersion on the server? (It will keep changing based on the client and the message). Here, can't we just use a loca…

vanchi-zendesk commented on pull request zendesk/radar#131
@vanchi-zendesk

Should we not replace everything here except socket id?

vanchi-zendesk commented on pull request zendesk/radar#131
@vanchi-zendesk

This can be a prototype method, so we can use this directly after we get the client, which I think is better. var c = Client.get(id, name, account)…

vanchi-zendesk commented on pull request zendesk/maxwell#47
@vanchi-zendesk

:+1:

vanchi-zendesk commented on pull request zendesk/radar#131
@vanchi-zendesk

Where is this defined?

vanchi-zendesk commented on pull request zendesk/radar#131
@vanchi-zendesk

likewise, here too. multi removes one turn-around time.

vanchi-zendesk commented on pull request zendesk/radar#131
@vanchi-zendesk

These things could be put into a multi, so it happens in a single shot.

vanchi-zendesk commented on pull request zendesk/radar#131
@vanchi-zendesk

accountName should be a parameter here maybe. Also, we could do the keyGet here if we have it (since it's a concern of the instance)

vanchi-zendesk commented on pull request zendesk/persistence#10
@vanchi-zendesk

:+1:

vanchi-zendesk commented on pull request zendesk/persistence#10
@vanchi-zendesk

same here.

vanchi-zendesk commented on pull request zendesk/persistence#10
@vanchi-zendesk

JSON parse these values?

vanchi-zendesk commented on pull request zendesk/radar_client#47
@vanchi-zendesk

:+1:

vanchi-zendesk commented on pull request zendesk/radar#130
@vanchi-zendesk

:+1:

vanchi-zendesk commented on pull request zendesk/radar#130
@vanchi-zendesk

Think you can change these two info's into debugs. Not strictly needed for debugging.