Search: Adds re-try mechanism while re-indexing#2583
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I don't think the proposed fix will actually work, when a real serialization failure occurs, since it will put the database transaction into a different state, so the retries will not actually execute.
We could take an entirely different approach and make indexer serialization failures impossible by introducing a per-table lock that's shared between all processes, so the application indexers skip writing changes to that table, while the full reindex is in progress for that same table. Although it's probably possible for the skipped entries to become desynced that way, so it's probably better that those serialization failures happen. If we delay indexing instead of skipping it, we could probably cause the serialization failure to always happen in a regular request, instead of during indexing, but it may slow down some requests significantly while indexing is in progress and we potentially risk the lock getting stuck if reindexing crashes.
| and sqlstate.startswith('40') | ||
| and attempt < REINDEX_MAX_ATTEMPTS | ||
| ): | ||
| index_log.info( |
There was a problem hiding this comment.
I'm not quite sure what will happen to the raw connection here with a real database error, since we no longer use a session.begin_nested in the indexer.
I think it will probably be in an invalid state and will refuse to execute any further queries. So this fix wouldn't actually help right now. I'm not sure if doing a session.execute('ROLLBACK') here would be safe, since we're on a different thread we should be getting our own session, but I'm not sure if that means we're necessarily also getting our own raw connection.
Maybe manually doing tx = session.begin_nested() before processing the indexer tasks and then calling tx.rollback() in this branch would work, but I'm not sure.
Your mocked test case doesn't exercise the details here properly, since you're completely bypassing the indexer, when you emit an exception, so to the database it still looks like you only emitted the query once, and that it succeeded.
I'm not sure if there is a robust way to test this. You might be able to trigger a real serialization failure reliably by using two threads and some events for synchronization.
There was a problem hiding this comment.
Savepoints/Nested transactions will probably not work, since the start of the main transaction will be the same, so the database will still consider the retry a serialization failure. We probably need to hope here that SQLAlchemy properly isolates the connections in the pool and that a manual session.execute('ROLLBACK') is safe.
| finally: | ||
| session.invalidate() | ||
| if session.bind and hasattr(session.bind, 'dispose'): | ||
| session.bind.dispose() |
There was a problem hiding this comment.
I kind of missed that this finally was happening inside the loop. It's a little bit wasteful using a new connection for each iteration, but it's probably fine for error recovery in this case.
This is also the only reason it was working, even though I thought it shouldn't, since invalidating the session and disposing of the connection implicitly rolls back the transaction and starts a new transaction in the next iteration.
You could try moving this out of the loop and changing the returns inside the loop to break and emitting an explicit session.execute(text('ROLLBACK')) before you continue.
You would need to be a little careful about BaseException for things like KeyboardInterrupt, but since we're tearing the entire application down in those cases, the connections should still get cleaned up.
A different approach, that preserves the old semantics of cleanup always happening, even with BaseException, would be wrapping the entire loop in a try: finally: block and move the cleanup into that finally block. That's what I thought was going on already.
2 participants
Search: Adds re-try mechanism while re-indexing
A failed re-indexing will surface as an error
TYPE: Feature
LINK: pro-1568