Clean up transaction management for file_complete handler #930
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BenGalewsky
force-pushed
the
file_complete_transaction
branch
from
70a6702 to
c2fb610
Compare
ponyisi
reviewed
Dec 5, 2024
servicex_app/servicex_app/resources/internal/transformer_file_complete.py
Show resolved
Hide resolved
servicex_app/servicex_app/resources/internal/transformer_file_complete.py
Outdated
Show resolved
Hide resolved
BenGalewsky
force-pushed
the
file_complete_transaction
branch
from
c2fb610 to
4a4ca9a
Compare
The celery transaction model settings to guarantee we never lose a file could result in duplicate file reports. Handle this by adding a unique key to the transformation_result table. Attempt to insert the record and just report a warning if that fails, but don't increment the file counter.
BenGalewsky
force-pushed
the
file_complete_transaction
branch
from
8ca7037 to
9ad4732
Compare
ponyisi
approved these changes
Jan 21, 2025
| @@ -456,7 +463,7 @@ def init(args: Union[Namespace, SimpleNamespace], app: Celery) -> None: | |||
| "--without-mingle", | |||
| "--without-gossip", | |||
| "--without-heartbeat", | |||
| "--loglevel=warning", | |||
| "--loglevel=info", | |||
There was a problem hiding this comment.
Won't this bring back a lot of uninteresting verbosity?
There was a problem hiding this comment.
The better to debug with til we are certain this is fixed -- I suppose it could be a helm chart setting, but that's a bit of a chore to work through
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
The TransformerFileComplete resource handler is the most critical code in the entire stack. It responds to each file in the dataset being transformed, and is responsible for updating the total number of files processed (either successfully, or failure) - these two counters are how we determine if the transform is complete. The endpoint will be hit repeatedly by all of the running transformers. Consequently, database transaction handing is very important to avoid missing files.
The current implementation uses implicit transactions and doesn't manage locks and flushing to the DB. It's possible that this allows for files to be lost during big transform requests.
Approach
record_file_completeto read the request withwith_for_updateflag set which will lock the record in the dbretrycall arguments are captured in a single, new decorator.file_complete_ops_retry-With this decorator, I ran into a problem with unit tests. Importing the module caused the
current_app.loggerexpression to be evaluated. This would throwRuntimeError: Working outside of application context.in the unit tests. Worked around this in the decorator to only access that logger if we are inside the flask app