Payday rewrite, part 5 #2579

Changaco · 2014-07-17T12:46:16Z

Step 6 of #2508. Follow-on from #2541.

chadwhitacre · 2014-07-29T17:41:16Z

I just ran the test suite for this branch. 449 passed, 2 xfailed in 27.92 seconds. 100% coverage on exchanges.py, 99% on payday.py. Looking good so far! :-)

fixes #1486

chadwhitacre · 2014-07-29T21:32:46Z

Hit an error:

[gittip] $ ./payday.sh foo
Rerun payday #foo? (y/N) y
Logging to ../paydays/test-foo.log.
[gittip] $ cat ../paydays/test-foo.log
Tue Jul 29 21:31:40 UTC 2014
pid-16089 thread-140735191843600 (MainThread) Traceback (most recent call last):
pid-16089 thread-140735191843600 (MainThread)   File "/Users/whit537/personal/gittip/www.gittip.com/gittip/cli.py", line 33, in payday
pid-16089 thread-140735191843600 (MainThread)     sync_with_balanced()
pid-16089 thread-140735191843600 (MainThread) TypeError: sync_with_balanced() takes exactly 1 argument (0 given)
[gittip] $

chadwhitacre · 2014-07-30T14:58:18Z

IRC re: integration testing. Decided to do it manually.

chadwhitacre · 2014-07-30T15:06:58Z

gittip/billing/payday.py

+                else:
+                    # The amount is too low, cancel the hold and make a new one
+                    cancel_card_hold(holds.pop(p.id))
+            hold, error = create_card_hold(self.db, p, amount)


create_card_hold and ach_credit use two different error models. They raise exceptions, and they also return an error value. Here we're only handling the second of these strategies, with the result that an exception raised by create_card_hold will terminate payday, whereas it should result in a log message.

It looks like in payout, which calls ach_credit, both error models are handled. We have a try except block in there around ach_credit.

No, it should terminate payday, create_card_hold is not supposed to raise an exception here, if it does then something is wrong, like the fact that I forgot to add AND is_suspicious IS false in the query above.

chadwhitacre · 2014-07-30T15:19:34Z

Should we change the few remaining uses of pachinko to takes?

chadwhitacre · 2014-07-30T15:38:51Z

Looks like we're running afoul of the database_maxconn restriction?

Wed Jul 30 15:36:22 UTC 2014
pid-17348 thread-140735191843600 (MainThread) Picking up with an existing payday.
pid-17348 thread-140735191843600 (MainThread) Payday started at 2014-07-30 14:58:49.849785+00:00.
pid-17348 thread-140735191843600 (MainThread) Greetings, program! It's PAYDAY!!!!
pid-17348 thread-140735191843600 (MainThread) Prepared the DB.
pid-17348 thread-140735191843600 (MainThread) Holding 3636 cents ($35.00 + $1.36 fee = $36.36) on Balanc
pid-17348 thread-140735191843600 (MainThread) Traceback (most recent call last):
pid-17348 thread-140735191843600 (MainThread)   File "/Users/whit537/personal/gittip/www.gittip.com/gitt
pid-17348 thread-140735191843600 (MainThread)     Payday.start().run()
pid-17348 thread-140735191843600 (MainThread)   File "/Users/whit537/personal/gittip/www.gittip.com/gitt
pid-17348 thread-140735191843600 (MainThread)     self.payin()
pid-17348 thread-140735191843600 (MainThread)   File "/Users/whit537/personal/gittip/www.gittip.com/gitt
pid-17348 thread-140735191843600 (MainThread)     self.settle_card_holds(cursor, holds)
pid-17348 thread-140735191843600 (MainThread)   File "/Users/whit537/personal/gittip/www.gittip.com/gitt
pid-17348 thread-140735191843600 (MainThread)     capture_card_hold(self.db, p, amount, holds.pop(p.id))
pid-17348 thread-140735191843600 (MainThread)   File "/Users/whit537/personal/gittip/www.gittip.com/gitt
pid-17348 thread-140735191843600 (MainThread)     e_id = record_exchange(db, 'bill', amount, fee, partic
pid-17348 thread-140735191843600 (MainThread)   File "/Users/whit537/personal/gittip/www.gittip.com/gitt
pid-17348 thread-140735191843600 (MainThread)     with db.get_cursor() as cursor:
pid-17348 thread-140735191843600 (MainThread)   File "/Users/whit537/personal/gittip/www.gittip.com/env/
pid-17348 thread-140735191843600 (MainThread)     self.conn = self.pool.getconn()
pid-17348 thread-140735191843600 (MainThread)   File "/Users/whit537/personal/gittip/www.gittip.com/env/
pid-17348 thread-140735191843600 (MainThread)     return self._getconn(key)
pid-17348 thread-140735191843600 (MainThread)   File "/Users/whit537/personal/gittip/www.gittip.com/env/
pid-17348 thread-140735191843600 (MainThread)     raise PoolError("connection pool exausted")
pid-17348 thread-140735191843600 (MainThread) PoolError: connection pool exausted

chadwhitacre · 2014-07-30T15:40:46Z

By commenting out the maxconn restriction I was able to run a payday locally! 💃

Wed Jul 30 15:39:22 UTC 2014
pid-17376 thread-140735191843600 (MainThread) Picking up with an existing payday.
pid-17376 thread-140735191843600 (MainThread) Payday started at 2014-07-30 14:58:49.849785+00:00.
pid-17376 thread-140735191843600 (MainThread) Greetings, program! It's PAYDAY!!!!
pid-17376 thread-140735191843600 (MainThread) Prepared the DB.
pid-17376 thread-140735191843600 (MainThread) Holding 3636 cents ($35.00 + $1.36 fee = $36.36) on Balanced for lgtest ... succeeded.
pid-17376 thread-140735191843600 (MainThread) Captured 3636 cents ($35.00 + $1.36 fee = $36.36) on Balanced for lgtest
pid-17376 thread-140735191843600 (MainThread) Captured 1 card holds.
pid-17376 thread-140735191843600 (MainThread) Canceled 0 card holds.
pid-17376 thread-140735191843600 (MainThread) Updated the balances of 3 participants.
pid-17376 thread-140735191843600 (MainThread) Starting payout loop.
pid-17376 thread-140735191843600 (MainThread) Crediting Gittip 2500 cents ($25.00 - $0.00 fee = $25.00) on Balanced ... succeeded.
pid-17376 thread-140735191843600 (MainThread) UNREVIEWED: whit537
pid-17376 thread-140735191843600 (MainThread) Did payout for 2 participants.
pid-17376 thread-140735191843600 (MainThread) Checked the DB.
pid-17376 thread-140735191843600 (MainThread) Updated payday stats.
pid-17376 thread-140735191843600 (MainThread) Updated receiving amounts.
pid-17376 thread-140735191843600 (MainThread) Script ran for {age} (0:00:05.311580).

Changaco · 2014-07-30T15:51:09Z

I don't think we really need the maxconn restriction so I've removed it.

chadwhitacre · 2014-07-30T15:56:15Z

@Changaco I'm pretty sure that statement_timeout only applies at the connection level. If we're using multiple connections during payday and we only set statement_timeout on one of them and we get a long query on a second connection then the we'll hit an error and payday will crash. If we think that all of our queries during payday will be below the default limit (30 seconds?) then we should remove the statement_timeout configuration entirely, to be added back once we find we need it again.

chadwhitacre · 2014-07-30T15:56:33Z

Docs for statement_timeout: http://www.postgresql.org/docs/9.3/static/runtime-config-client.html.

chadwhitacre · 2014-07-30T15:57:15Z

#1600 has backstory on statement_timeout.

chadwhitacre · 2014-07-30T16:07:51Z

I'm trying to see what the current setting for statement_timeout is. At #1541 (comment) I say that I set it to 10 s, but I don't say how I did that. select * from pg_settings where name='statement_timeout'; shows 0. Did I configure this in a different (per-user?) place? Did we clobber the setting during a db upgrade?

Changaco · 2014-07-30T16:08:44Z

Should we make the error handling parallel in payout? That is, can we do away with the try/except block in there around ach_credit?

We can, but we'll lose the UNREVIEWED: log messages.

No test?

We can add one of course, I simply forgot.

chadwhitacre · 2014-07-30T16:11:18Z

An UPDATE applied to a row of pg_settings is equivalent to executing the SET command on that named parameter. The change only affects the value used by the current session.

http://www.postgresql.org/docs/9.3/static/view-pg-settings.html

chadwhitacre · 2014-07-30T16:16:19Z

Not seeing statement_timeout in select rolname, rolconfig from pg_roles; either. :-/

Reticketed as #2601.

chadwhitacre · 2014-07-30T16:19:05Z

We can, but we'll lose the UNREVIEWED: log messages.

Did we lose those message on the payin side?

Changaco · 2014-07-30T16:48:52Z

Did we lose those message on the payin side?

I'm not sure.

Changaco · 2014-07-30T16:58:09Z

On second thought it's trivial to keep the log message, I've added a commit that removes the try/except block but keeps the log message.

chadwhitacre · 2014-07-30T17:45:56Z

On second thought it's trivial to keep the log message, I've added a commit that removes the try/except block but keeps the log message.

Awesome, good call!

chadwhitacre · 2014-07-30T18:12:22Z

I simulated a payday crash and ended up with a balance conflict when rerunning. Sorry, I don't have more detailed information about where in the payday cycle the crash hit.

Wed Jul 30 17:51:20 UTC 2014
pid-18561 thread-140735191843600 (MainThread) Starting a new payday.
pid-18561 thread-140735191843600 (MainThread) Payday started at 2014-07-30 17:51:21.438705+00:00.
pid-18561 thread-140735191843600 (MainThread) Greetings, program! It's PAYDAY!!!!
pid-18561 thread-140735191843600 (MainThread) Prepared the DB.
pid-18561 thread-140735191843600 (MainThread) Holding 3636 cents ($35.00 + $1.36 fee = $36.36) on Balanced for lgtest ... succeeded.
pid-18561 thread-140735191843600 (MainThread) Captured 3430 cents ($33.00 + $1.30 fee = $34.30) on Balanced for lgtest
pid-18561 thread-140735191843600 (MainThread) Captured 1 card holds.
pid-18561 thread-140735191843600 (MainThread) Canceled 0 card holds.
pid-18561 thread-140735191843600 (MainThread) Updated the balances of 3 participants.
pid-18561 thread-140735191843600 (MainThread) Starting payout loop.
pid-18561 thread-140735191843600 (MainThread) UNREVIEWED: whit537
Traceback (most recent call last):
  File "/Users/whit537/personal/gittip/www.gittip.com/env/bin/honcho", line 9, in <module>
    load_entry_point('honcho==cfcc7ebb', 'console_scripts', 'honcho')()
  File "/Users/whit537/personal/gittip/www.gittip.com/env/lib/python2.7/site-packages/honcho/command.py", line 241, in main
    args.func(args)
  File "/Users/whit537/personal/gittip/www.gittip.com/env/lib/python2.7/site-packages/honcho/command.py", line 169, in command_run
    p.wait()
  File "/Users/whit537/lib/python2.7/subprocess.py", line 1357, in wait
    pid, sts = _eintr_retry_call(os.waitpid, self.pid, 0)
  File "/Users/whit537/lib/python2.7/subprocess.py", line 478, in _eintr_retry_call
    return func(*args)
KeyboardInterrupt

Wed Jul 30 17:53:34 UTC 2014
pid-18582 thread-140735191843600 (MainThread) Picking up with an existing payday.
pid-18582 thread-140735191843600 (MainThread) Payday started at 2014-07-30 17:51:21.438705+00:00.
pid-18582 thread-140735191843600 (MainThread) Traceback (most recent call last):
pid-18582 thread-140735191843600 (MainThread)   File "/Users/whit537/personal/gittip/www.gittip.com/gittip/cli.py", line 28, in payday
pid-18582 thread-140735191843600 (MainThread)     Payday.start().run()
pid-18582 thread-140735191843600 (MainThread)   File "/Users/whit537/personal/gittip/www.gittip.com/gittip/billing/payday.py", line 102, in run
pid-18582 thread-140735191843600 (MainThread)     self.db.self_check()
pid-18582 thread-140735191843600 (MainThread)   File "/Users/whit537/personal/gittip/www.gittip.com/gittip/models/__init__.py", line 17, in self_check
pid-18582 thread-140735191843600 (MainThread)     check_db(cursor)
pid-18582 thread-140735191843600 (MainThread)   File "/Users/whit537/personal/gittip/www.gittip.com/gittip/models/__init__.py", line 23, in check_db
pid-18582 thread-140735191843600 (MainThread)     _check_balances(cursor)
pid-18582 thread-140735191843600 (MainThread)   File "/Users/whit537/personal/gittip/www.gittip.com/gittip/models/__init__.py", line 96, in _check_balances
pid-18582 thread-140735191843600 (MainThread)     assert b == 0, "conflicting balances: {}".format(b)
pid-18582 thread-140735191843600 (MainThread) AssertionError: conflicting balances: 1

chadwhitacre · 2014-07-30T18:18:22Z

Why name the temporary tables pay_foo instead of payday_foo?

Changaco · 2014-07-30T18:29:56Z

I'm not sure why, but I can rename them if you want.

Changaco · 2014-07-30T18:36:12Z

I simulated a payday crash and ended up with a balance conflict when rerunning. Sorry, I don't have more detailed information about where in the payday cycle the crash hit.

Hmm, can you dump the DB so that I can inspect the participants and exchanges?

Changaco · 2014-07-31T10:06:40Z

@whit537 I believe I've found the problem, I've added commits to fix it and prevent future problems from going undetected like this one.

chadwhitacre · 2014-07-31T18:01:58Z

@Changaco I think we should write a script to populate a Balanced marketplace with users and credit cards and bank accounts according to last_{ach,bill}_result in a DB backup. I'm willing to work on that when I get back to this next week.

chadwhitacre · 2014-07-31T18:02:44Z

https://docs.balancedpayments.com/1.1/overview/resources/#test-credit-card-numbers
https://docs.balancedpayments.com/1.1/overview/resources/#test-bank-account-numbers

Changaco · 2014-08-11T21:28:55Z

@Changaco Did you address this and I missed it?

No, I forgot to answer that. I'm not sure it's a good idea to stop withholding money for the following week. I think we should keep the current behavior for now.

Okay, but without that condition we'd end up with negative balances, right? That condition is what ensures that we pass over some tips, even while we accept further downstream tips.

Yes.

chadwhitacre · 2014-08-11T21:46:58Z

I'm not sure it's a good idea to stop withholding money for the following week. I think we should keep the current behavior for now.

Hmm ... this was one of the driving concerns on #1486. What's the reason not to make that change here?

Changaco · 2014-08-12T07:57:15Z

Hmm ... this was one of the driving concerns on #1486. What's the reason not to make that change here?

#1486 is not about holding escrow, it's about unnecessarily high credit card charges, and that's fixed in the payday rewrite by implementing card holds. You're probably thinking of #1383.

closes #2441

chadwhitacre · 2014-08-12T11:43:10Z

@Changaco #1486 quotes from Twitter:

A little math upfront would not only lessen fees, but help avoid the 52K in escrow, which feels like a scam.

Changaco · 2014-08-12T11:50:48Z

The withholding mechanism is only responsible for $573.75 of our current escrow.

select sum(balance) from participants where last_ach_result is not null and balance = giving+pledging;

chadwhitacre · 2014-08-12T11:54:50Z

The withholding mechanism is only responsible for $573.75 of our current escrow.

Okay, fair enough. I guess if I'm honest, I was personally hoping to get that $150 I've got stuck in Gittip right now, but that's a bad basis for this decision. :-)

chadwhitacre · 2014-08-12T11:55:17Z

Okay! I think we might be close, @Changaco! 💃 What's left to do here?

Changaco · 2014-08-12T11:58:28Z

@whit537 Review #2610 and #2620 and merge them into this one. You could also give another try to #2611.

Total runtime: 52570.561 ms → 118.837 ms

Optimize DB check

Payday rewrite, part 5

chadwhitacre · 2014-08-12T16:09:49Z

!m @Changaco

Changaco added 4 - Review labels Jul 29, 2014

chadwhitacre referenced this pull request Jul 29, 2014

rewrite the payin loop in SQL

afc27b4

fixes #1486

chadwhitacre reviewed Jul 30, 2014
View reviewed changes

chadwhitacre mentioned this pull request Jul 30, 2014

bring back statement_timeout? #2601

Closed

Changaco and others added 12 commits August 12, 2014 10:40

better error message when exchange fails

c494868

closes #2441

fix test

7c610ff

update test to detect another bug

24db90b

fix payin() to flag which participants we actually have card holds for

8297378

fix minor bug in update_stats()

1e0a0a7

Let's put the populate-balanced.py script in git

e8ce300

fix UPDATE query in prepare()

1672083

failing tests for NoResultFound()

b7b6046

get rid of NoResultFound()

e952383

Update branch.py to work with a cache

a6d6cff

rename SQL tables pay_* → payday_*

9a28447

replace DROP TABLE IF EXISTS by ON COMMIT DROP

f27a952

Changaco and others added 3 commits August 12, 2014 14:27

zero out known anomalous tip

4b61dbd

optimize DB check

26ea085

Total runtime: 52570.561 ms → 118.837 ms

Merge pull request #2610 from gittip/optimize-db-check

5c42c57

Optimize DB check

chadwhitacre added a commit that referenced this pull request Aug 12, 2014

Merge pull request #2579 from gittip/payday-rewrite-5

e9d037d

Payday rewrite, part 5

chadwhitacre merged commit e9d037d into master Aug 12, 2014

This was referenced Aug 14, 2014

run Gittip 115 #2634

Closed

settle the whole graph of tips, not just the first part #2664

Closed

chadwhitacre mentioned this pull request Aug 21, 2014

run Gittip 116 #2667

Closed

chadwhitacre mentioned this pull request Oct 16, 2014

run Gratipay 124 #2814

Closed

Payday rewrite, part 5 #2579

Payday rewrite, part 5 #2579

Conversation

Changaco commented Jul 17, 2014

chadwhitacre commented Jul 29, 2014

chadwhitacre commented Jul 29, 2014

chadwhitacre commented Jul 30, 2014

chadwhitacre Jul 30, 2014

Choose a reason for hiding this comment

chadwhitacre Jul 30, 2014

Choose a reason for hiding this comment

Changaco Jul 30, 2014

Choose a reason for hiding this comment

chadwhitacre commented Jul 30, 2014

chadwhitacre commented Jul 30, 2014

chadwhitacre commented Jul 30, 2014

Changaco commented Jul 30, 2014

chadwhitacre commented Jul 30, 2014

chadwhitacre commented Jul 30, 2014

chadwhitacre commented Jul 30, 2014

chadwhitacre commented Jul 30, 2014

Changaco commented Jul 30, 2014

chadwhitacre commented Jul 30, 2014

chadwhitacre commented Jul 30, 2014

chadwhitacre commented Jul 30, 2014

Changaco commented Jul 30, 2014

Changaco commented Jul 30, 2014

chadwhitacre commented Jul 30, 2014

chadwhitacre commented Jul 30, 2014

chadwhitacre commented Jul 30, 2014

Changaco commented Jul 30, 2014

Changaco commented Jul 30, 2014

Changaco commented Jul 31, 2014

chadwhitacre commented Jul 31, 2014

chadwhitacre commented Jul 31, 2014

Changaco commented Aug 11, 2014

chadwhitacre commented Aug 11, 2014

Changaco commented Aug 12, 2014

chadwhitacre commented Aug 12, 2014

Changaco commented Aug 12, 2014

chadwhitacre commented Aug 12, 2014

chadwhitacre commented Aug 12, 2014

Changaco commented Aug 12, 2014

chadwhitacre commented Aug 12, 2014