Remove cache flushing on startup

[rijkvanzanten]

This was added way back when to make sure that projects that upgrade don't run into bugs due to changes in the structure of the cache. However, this comes with the (huge) side-effect that all caches are flushed whenever a horizontal scaling event happens (eg when the project is scaling up, the caches are flushed, putting needless strain on the database while all containers try to regenerate the schema cache simultaneously).

Fixes #18245

[paescuj]

Should we instead flush the caches in api/src/database/migrations/run.ts in case a migration has actually been performed? Theoretically a migration can lead to any changes in the database.

[licitdev]

Should we instead flush the caches in api/src/database/migrations/run.ts in case a migration has actually been performed? Theoretically a migration can lead to any changes in the database.

Yes we should. 👍 However, when a new migration is added or when the schema is changed, the same issue occurs.

I wonder if we should add a MAX_CONCURRENT_SCHEMA_FETCH using the messenger Redis connection where a schema_fetch_count key increments before the fetching takes place. 🤔

[paescuj]

Yes we should. 👍 However, when a new migration is added or when the schema is changed, the same issue occurs.

I wonder if we should add a MAX_CONCURRENT_SCHEMA_FETCH using the messenger Redis connection where a schema_fetch_count key increments before the fetching takes place. 🤔

Right, this sounds like a good plan 👍

[rijkvanzanten]

Should we instead flush the caches in api/src/database/migrations/run.ts in case a migration has actually been performed?

I don't think that'll solve it either. On migrate, you'll still have active containers on the old version, while the new one is upgrading. I think the only real fix is to make sure we add the current api version as part of the redis key

[paescuj]

Should we instead flush the caches in api/src/database/migrations/run.ts in case a migration has actually been performed?

I don't think that'll solve it either. On migrate, you'll still have active containers on the old version, while the new one is upgrading. I think the only real fix is to make sure we add the current api version as part of the redis key

Wouldn't the other instances get notified by the messenger about the schema change? Together with @licitdev's idea of locking the schema fetch I think that should work 🤔

Edit:

current api version as part of the redis key

Oh yeah, and for the instances still running on older versions this would be required too 👍

[paescuj]

For now I think we could simply move the flushCaches call to the migrations (of course only execute it if migrations are actually performed) and track the ideas discussed above in a separate issue. This way we could already solve the issue when it comes to scaling without upgrading instances.

[rijkvanzanten]

For now I think we could simply move the flushCaches call to the migrations (of course only execute it if migrations are actually performed)

@paescuj I don't think that'll work. Lets say you have 4 instances on v1. When updating, you'd move one instance to v2, wait for it to be done, and then move 2/4 to v2, 3/4 to v2, 4/4 to v2. When the first v2 comes online, migrations run and caches are flushed, however, because those other 3/4 are still on v1, the caches are recreated as fast as they're flushed. If there's a breaking change in that migration, it'll start crashing on both sides

[paescuj]

For now I think we could simply move the flushCaches call to the migrations (of course only execute it if migrations are actually performed)

@paescuj I don't think that'll work. Lets say you have 4 instances on v1. When updating, you'd move one instance to v2, wait for it to be done, and then move 2/4 to v2, 3/4 to v2, 4/4 to v2. When the first v2 comes online, migrations run and caches are flushed, however, because those other 3/4 are still on v1, the caches are recreated as fast as they're flushed. If there's a breaking change in that migration, it'll start crashing on both sides

Yeah, I understand. (So that would have already been the case up to now...? 👀😅)

[paescuj]

LGTM!