Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v1.5.11: active-orders endpoints [BAC-2110] #149

Merged
merged 1 commit into from
Dec 30, 2021
Merged

Conversation

samweinberg23
Copy link
Contributor

No description provided.

@linear
Copy link

linear bot commented Dec 30, 2021

BAC-2110 Librarian timing out on grabbing a connection to the Postgres DB

https://app.bugsnag.com/dydx/librarian/errors/6109a0aff7dd340008c20d28?filters[event.since]=30d&filters[error.status]=open

We've been experiencing a huge uptick in knex timeouts in Librarian for our primary DB:

  • as a result of too many connections.
  • We have since moved over more of our DB reads to a dedicated Read-Replica so that Librarian will maintain fewer concurrent connections with the primary DB.
  • A number of endpoints cannot be moved over since they are very dependent on having the most up to date and accurate information and the RR can lag and has no guarantee of being up to date.

One endpoint that sees a lot of traffic is POST v3/orders . Within that endpoint, replacing requires 2x the DB accesses as just posting an order. Replacing orders requires writing to the DB and also querying the order that is going to be canceled to make sure it exists and can be canceled for the account.

We frequently receive an error associated with the knex timeout:

| failed to get cancel order by id |

which indicates the timeout occurred while trying to query for the order to cancel

We can optimize and increase replace reliability by validating the cancel order against our ActiveOrders redis cache:

  • Breaking Change: Active orders are fetched by (orderId, accountId, market, side)so we will only be able to replace orders for the same market and side going forward. Note, if we are ok with making multiple redis calls and iterating over all Hashes of Market + Side we could still replace orders for market and side that differ from the new order's market and side. This is because, without a guarantee of market + side for the order being replaced, we must instead iterate over every market + side for the account. However, I think it is best to not overly complicate the code flow and also avoid scaling the cost to replace an order with the number of new markets we have. Therefore, I believe we should just limit replacing orders to the market + side of the new order being placed

  • Breaking Change: Active orders are stored as a subset of information about the order. Therefore, we will only have price, remainingSize, id, accountId, market and side for the order being canceled going forward and the response we would send for the cancelOrder in the response object would change from:

    {
        id,
        clientId,
        accountId,
        market,
        side,
        price,
        triggerPrice,
        trailingPercent,
        size,
        remainingSize,
        type,
        createdAt,
        unfillableAt,
        expiresAt,
        status,
        timeInForce,
        postOnly,
        cancelReason,
      }
    

to

{
id,
accountId,
market,
side,
remainingSize,
price,
}

Future

If we continue to see knex timeouts there are additional changes that can be made and some are being made in parallel to this ticket:

  1. We require market and side for DELETE v3/orders/id we can then utilize the active orders cache like we do for replace
  2. Move additional GET endpoint queries over to the RR

@samweinberg23 samweinberg23 merged commit 00f7791 into master Dec 30, 2021
@samweinberg23 samweinberg23 deleted the sw_active branch December 30, 2021 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants