This repository has been archived by the owner. It is now read-only.

Rewrite payday #2508

Closed
Changaco opened this Issue Jun 18, 2014 · 8 comments

Comments

Projects
None yet
2 participants
@Changaco
Contributor

Changaco commented Jun 18, 2014

Opening an issue as discussed in IRC.

Here are the main goals of the rewrite:

  • make it fast: having it run in under 10 minutes would be nice
  • make it safe: we've had failures in the past, we need to do better, it's money we're handling here
  • reduce the amounts of fees and escrow by not charging credit cards when we don't actually need to

And here's my checklist:

  • solve #1486 by implementing card holds
  • do stuff directly in SQL instead of making a lot of round trips between the DB and the python code:
    • update stats in a single request at the end of payday
    • rewrite the transfer of tips and takes in SQL
  • optimize the SQL (explain analyze FTW)
  • do payin in a single transaction
  • fix #1853
  • make sure we don't get out of sync with Balanced (#213)
  • make sure we don't transfer tips to participants who don't want to receive them (#916)
  • investigate #2537
  • handle payout failures (#1811)
  • parallelize the Balanced API requests

(Am I missing other issues that the payday rewrite should fix ?)

@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
@chadwhitacre

chadwhitacre Jun 18, 2014

Contributor

do everything in a single transaction

Why? Not doing everything in a single db transaction was an intentional design decision to enable better parallelization and crash recovery.

Contributor

chadwhitacre commented Jun 18, 2014

do everything in a single transaction

Why? Not doing everything in a single db transaction was an intentional design decision to enable better parallelization and crash recovery.

@Changaco

This comment has been minimized.

Show comment
Hide comment
@Changaco

Changaco Jun 18, 2014

Contributor

But payday is not actually parallelized, and its crash recovery logic has failed in the past. I'm not even sure payday could actually be parallelized. However I think it is possible to make it fast enough so that restarting from the start after a crash isn't be a problem.

Contributor

Changaco commented Jun 18, 2014

But payday is not actually parallelized, and its crash recovery logic has failed in the past. I'm not even sure payday could actually be parallelized. However I think it is possible to make it fast enough so that restarting from the start after a crash isn't be a problem.

@Changaco

This comment has been minimized.

Show comment
Hide comment
@Changaco

Changaco Jul 29, 2014

Contributor

I have rebased the payday rewrite branches on master, and I think they're ready for final review now.

Contributor

Changaco commented Jul 29, 2014

I have rebased the payday rewrite branches on master, and I think they're ready for final review now.

@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
@chadwhitacre

chadwhitacre Jul 29, 2014

Contributor

Awesome, @Changaco, looking at them now. IRC

Contributor

chadwhitacre commented Jul 29, 2014

Awesome, @Changaco, looking at them now. IRC

@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
@chadwhitacre

chadwhitacre Jul 29, 2014

Contributor

@Changaco Does it make sense to address #2443 in the rewrite?

Contributor

chadwhitacre commented Jul 29, 2014

@Changaco Does it make sense to address #2443 in the rewrite?

@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
@chadwhitacre

chadwhitacre Jul 29, 2014

Contributor

@Changaco It looks like 97aacd0 is close to #2443, anyway.

Contributor

chadwhitacre commented Jul 29, 2014

@Changaco It looks like 97aacd0 is close to #2443, anyway.

@Changaco

This comment has been minimized.

Show comment
Hide comment
@Changaco

Changaco Jul 30, 2014

Contributor

@whit537

It looks like 97aacd0 is close to #2443, anyway.

It was not my intention to fix #2443 in that commit, but it's true that it creates a one-to-one link between Gittip's exchanges and Balanced's transactions by inserting exchange_id in the meta field of the transaction. Is that sufficient to close #2443 or do we need more?

Contributor

Changaco commented Jul 30, 2014

@whit537

It looks like 97aacd0 is close to #2443, anyway.

It was not my intention to fix #2443 in that commit, but it's true that it creates a one-to-one link between Gittip's exchanges and Balanced's transactions by inserting exchange_id in the meta field of the transaction. Is that sufficient to close #2443 or do we need more?

@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
@chadwhitacre

chadwhitacre Jul 31, 2014

Contributor

Hmm ... could be. I think we'll end up wanting to store some information locally to avoid expensive HTTP calls for things like #1681. No?

Contributor

chadwhitacre commented Jul 31, 2014

Hmm ... could be. I think we'll end up wanting to store some information locally to avoid expensive HTTP calls for things like #1681. No?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.