Remove redudant db commit when getting serialised service

[spatel033]

We have investigated P1 504 outage and came across following possible issue. This PR is to remove db commit which was added previously to close the transaction to improve the performance.

First we get redis error - kibana

then pg connection closed error - kibana

Possible scenario we found during the P1 504 outage:

• Redis get disconnected
• We have this service bombarding auth request (200-450 auth request per min) with incorrect token
• All these requests now go to postgres
• When getting serialised service to get api_key for auth, it also does unnecessary db.session.commit() ??
• At the same time RDS has 66% commit queries

[leohemsted]

this was introduced here: 46dbcb7

Commit transactions as soon no longer needed
We think that holding open database transactions while we go and do something else is causing us to have poor performance.

Because we’re not serialising everything as soon as we pull it out of the database we can guarantee that we don’t need to go back to the database again.

So let’s see if explicitly closing the transaction helps with performance.

concurrent db connection count is not as big a concern for us right this second as it used to be

[quis]

If it makes sense to remove this we should also remove the one for SerialisedTemplate here:

notifications-api/app/serialised_models.py

Line 70 in 84e7981

db.session.commit()

…and SerialisedAPIKeys here:

notifications-api/app/serialised_models.py

Line 130 in 84e7981

db.session.commit()

But (without a description in your PR) I’m not understanding the benefit/rationale for doing this now.

[leohemsted]

i'm still a little skeptical about this:

1. i dont understand why redis would fail - we havent had redis errors before/elsewhere
2. the redis errors we see are EventletTimeoutErrors. There are two reasons an eventlet could time out:

• the IO takes over thirty seconds
• the IO completed on time but the eventlet process wasn't able to carry on with the code execution in under 30 seconds

1. i dont see why this would be contained to one specific api-web instance
2. 450 requests per min sounds like a lot but it's only 7.5 requests per second - whereas we have 35 concurrent instances with four processes each that can each handle 250 requests

[spatel033]

i'm still a little skeptical about this:

1. i dont understand why redis would fail - we havent had redis errors before/elsewhere
2. the redis errors we see are EventletTimeoutErrors. There are two reasons an eventlet could time out:

• the IO takes over thirty seconds
• the IO completed on time but the eventlet process wasn't able to carry on with the code execution in under 30 seconds

1. i dont see why this would be contained to one specific api-web instance
2. 450 requests per min sounds like a lot but it's only 7.5 requests per second - whereas we have 35 concurrent instances with four processes each that can each handle 250 requests

Yes this PR will not solve the actual redis issue but it may stop 504 error when redis connection is closed

[spatel033]

i'm still a little skeptical about this:

1. i dont understand why redis would fail - we havent had redis errors before/elsewhere
2. the redis errors we see are EventletTimeoutErrors. There are two reasons an eventlet could time out:

• the IO takes over thirty seconds
• the IO completed on time but the eventlet process wasn't able to carry on with the code execution in under 30 seconds

1. i dont see why this would be contained to one specific api-web instance
2. 450 requests per min sounds like a lot but it's only 7.5 requests per second - whereas we have 35 concurrent instances with four processes each that can each handle 250 requests

Yes this PR will not solve the actual redis issue but it may stop 504 error when redis connection is closed

Recent P1 stared with eventlet timeout for SerialisedAPIKeyCollection.from_service_id(self.id)