Bulk insert potential payments #3375
Conversation
We want to bulk insert these for performance reasons. We no longer send out emails and with this, we will no longer sync connection information for each of the wallets. That is done when a user logs in. This lets us delete and simplify the code. I'm sure there are things that can still be done, and might be -- but this is a first stab. Things to be done for this PR before merge: - tests pass - Make sure all records loaded up front, so no n+1 Things that still might be done, but maybe for v2: 1) Eliminate the create_message calls and just return the objects for bulk insertion 2) Make uphold_service and gemini_service style like bitflyer_service
app/models/publisher.rb
Outdated
| @@ -112,8 +112,7 @@ class Publisher < ApplicationRecord | |||
| ############################### | |||
|
|
|||
| scope :uphold_selected_provider, -> { | |||
| joins("INNER JOIN uphold_connections | |||
| ON uphold_connections.id = publishers.selected_wallet_provider_id | |||
| where("uphold_connections.id = publishers.selected_wallet_provider_id | |||
There was a problem hiding this comment.
Unless I'm mistaken, scopes like these should still work in isolation.
There was a problem hiding this comment.
Yeah these are only ever called from our job, and with eager loading it throws a double reference error to have the join here. I'll work on a workaround.
There was a problem hiding this comment.
So the confusing part is that below, you're using both includes and joins. Is it possible to revert back to the original definition of uphold_selected_provider, and if not, why?
There was a problem hiding this comment.
You need to use includes for eager loading since we enforce strict loading, but we need to joins for filtering.
|
Would you make it so that the code is idempotent/retryable? If the primary jobs fails part way, it doesn't seem like there's a way to restart from the middle |
| # Implemented in Payout::Service | ||
| return if skip_publisher? | ||
| class GeminiService | ||
| def perform(payout_report:, publisher:) |
There was a problem hiding this comment.
These were part of the same Payout::Service class to use the same initializer so we didn't have to repeat ourselves, may we revert back?
There was a problem hiding this comment.
This is to bring these in line with the Bitflyer Service class for consistency. The methods in that class are utility classes and can be called as such; there's no need to bind these classes together in a hierarchy -- they can each just call the helper. Having it use inheritance causes a lot of pain when trying to add the Bitflyer Service class due to the complexity in that class, so we began refactoring it then. This is the completion of that work.
Co-authored-by: Albert Wang <amwang217@gmail.com>
Co-authored-by: Albert Wang <amwang217@gmail.com>
tsmartt
force-pushed
the
bulk-insert-potential-payments
branch
from
55a6ed7
to
db7cb2f
Compare
app/services/payout/service.rb
Outdated
| PayoutMessage.create( | ||
| payout_report: @payout_report, | ||
| publisher: @publisher, | ||
| message: "#{self.class.name} #{message}" | ||
| message: "#{@class_name} #{message}" |
There was a problem hiding this comment.
Rather than passing by argument, the child class inherits this function so refactoring was unnecessary.
There was a problem hiding this comment.
I prefer composition, you inheritance here. I'll make the change.
Initial stab at simplifying potential payments
We want to bulk insert these for performance reasons. We no longer send
out emails and with this, we will no longer sync connection information
for each of the wallets. That is done when a user logs in.
This lets us delete and simplify the code. I'm sure there are things
that can still be done, and might be -- but this is a first stab.
Things to be done for this PR before merge:
Things that still might be done, but maybe for v2:
for bulk insertion