search

artsy / team-navigator

An internal HR product for Artsy's team
https://team.artsy.net
MIT License
62 stars 19 forks source link

@dblock: Fix state bug when using a single instance of the CSV parser #88

Closed craigspaeth closed 7 years ago

craigspaeth commented 7 years ago

We're using csvtojson to convert the spreadsheet into members. Apparently, it is an issue to use the same instance of that converter, so this creates a new instance for every sync we run.

dblock commented 7 years ago

So the sync would run once but not the second time? Why didn't a restart of the app fix the problem?

craigspaeth commented 7 years ago

A restart did fix it, but running sync again would get the server stuck in the bad state. This was extra problematic because the first thing we do in sync is db.members.drop(), so running sync a second time would successfully drop the members collection and then get stuck on the CSV parsing—leaving the app with an emptied database.

dblock commented 7 years ago

Is the sync setup on a cron? Do those failures get reported to someone somewhere?

craigspaeth commented 7 years ago

From what I can tell that sync is just run manually via GraphQL with errors reported in the GraphQL response. @orta Might know more.

orta commented 7 years ago

I do, yep, the sync mutation is the only place it's happening. I ended up removing the chron jobs

dblock commented 7 years ago

Could someone please explain to me like a 2yo when and why we need this sync and why it's not on a cron?

orta commented 7 years ago

Sure, this uses the new Web Stack Craig made ( MuralJS )

It uses GraphQL to handle all of the API architecture, the mutation we are talking about is using that. A mutation is the equivalent of a POST/PUT but that it also collects the side-effects of the changes from the data mutation (more here)

As this is the only MuralJS project I didn't want to be skipping the actual Mural paradigm and just write a separate sync.

I originally had a daily sync on a cron job (just a CURL to that API), but I removed it as I went OOO for 3 weeks and valued stability the team-nav. I moved a bunch of the cron jobs inline in #84 but had't got around to re-automating running the sync on a daily process in heroku.

craigspaeth commented 7 years ago

Thanks Orta!

@dblock I'll add that this spreadsheet syncing is a pretty un-ideal solution, cron job or not. We discussed with People Ops eventually adopting a third party tool for this in the long term—or if we do continue with this in-house app, then IMO we should be using a better data model with some admin UI and not syncing over Google spreadsheets.