Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

Settle whole tip graph (payday) #2914

Merged
merged 6 commits into from
Dec 4, 2014
Merged

Conversation

rohitpaulk
Copy link
Contributor

fixes #2664

@rohitpaulk rohitpaulk changed the title Fix whole tip graph payday Settle whole tip graph (payday) Nov 9, 2014
@rohitpaulk rohitpaulk force-pushed the fix-whole-tip-graph-payday branch from ffdb729 to 7105d6d Compare November 9, 2014 12:05
@Changaco
Copy link
Contributor

Changaco commented Nov 9, 2014

Not bad, but to settle the whole graph you also need to run the tips loop after the takes have been distributed. Also I don't understand what that discovered column is.

@rohitpaulk
Copy link
Contributor Author

Not bad, but to settle the whole graph you also need to run the tips loop after the takes have been distributed.

True. Which makes me think - The second UPDATE statement should be run as a loop too.

Also I don't understand what that discovered column is.

The discovered column is an attempt to improve performance by reducing the number of times the process_tip() function runs. I try to follow the flow of money - starting from people with credit cards attached. Each person they send a tip to, is marked as discovered. After that, the is_funded update is done only on people who are discovered. Again, each person who receives a tip is marked as discovered and the loop runs again. If it weren't for this we'd be trying to update is_funded every time on people who are far far down the line, which would result in process_tip() being called unnecessarily in cases where we know for sure that is_funded is not going to change to true.

I don't know how much this will affect performance, and I guess it could be done away with for the sake of simplicity.

What do you think @Changaco ?

@Changaco
Copy link
Contributor

Changaco commented Nov 9, 2014

I think you should remove discovered. I also think the tip loop should be a function that we EXECUTE before and after takes.

@Changaco
Copy link
Contributor

Changaco commented Dec 4, 2014

Ping @rohitpaulk.

@rohitpaulk
Copy link
Contributor Author

@Changaco - I saw this, looks good to me. Didn't want to merge coz it's my own PR. If you signal green on this, let's merge it in.

@Changaco
Copy link
Contributor

Changaco commented Dec 4, 2014

@rohitpaulk You started the PR but your commits have already been reviewed, so you could have merged it after reviewing my commits. Or you could have just said "looks good to me" and I would have merged, but if you don't say anything I can't know that you've already reviewed the commits.

Changaco added a commit that referenced this pull request Dec 4, 2014
@Changaco Changaco merged commit e63f9b8 into master Dec 4, 2014
@Changaco Changaco deleted the fix-whole-tip-graph-payday branch December 4, 2014 11:15
@rohitpaulk
Copy link
Contributor Author

@Changaco - True, will keep that in mind :)

@chadwhitacre
Copy link
Contributor

!m @rohitpaulk @Changaco

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

Successfully merging this pull request may close these issues.

settle the whole graph of tips, not just the first part
3 participants