-
Notifications
You must be signed in to change notification settings - Fork 557
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
delegated operator fixes #4908
delegated operator fixes #4908
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -262,6 +262,8 @@ def update_run_state( | |
else None | ||
) | ||
|
||
needs_pipeline_update = False | ||
|
||
if run_state == ExecutionRunState.COMPLETED: | ||
update = { | ||
"$set": { | ||
|
@@ -272,10 +274,11 @@ def update_run_state( | |
} | ||
} | ||
|
||
if outputs_schema: | ||
update["$set"]["metadata.outputs_schema"] = { | ||
"$ifNull": [outputs_schema, {}] | ||
} | ||
if outputs_schema: | ||
update["$set"]["metadata.outputs_schema"] = ( | ||
outputs_schema or {} | ||
) | ||
needs_pipeline_update = True | ||
|
||
elif run_state == ExecutionRunState.FAILED: | ||
update = { | ||
|
@@ -325,9 +328,15 @@ def update_run_state( | |
if required_state is not None: | ||
collection_filter["run_state"] = required_state | ||
|
||
# Using pipeline update instead of a single update doc fixes a case | ||
# where `metadata` is null and so accessing the dotted field | ||
# `metadata.output_schema` creates the document instead of erroring. | ||
if needs_pipeline_update: | ||
update = [update] | ||
|
||
doc = self._collection.find_one_and_update( | ||
filter=collection_filter, | ||
update=[update], | ||
update=update, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this should be an array (aggregation pipeline). Otherwise passing a dictionary will be interpreted as the new doc so you'd essentially be setting the doc to have a new field "$set" with value equal to the dictionary value There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may be thinking of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternatively if you don't specify There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://www.mongodb.com/docs/manual/reference/method/db.collection.findOneAndUpdate/ I'm just going off of the mongo docs... Since the change didn't require any corresponding test changes, are there tests that would catch this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But yeah the alternative is to remove set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have to go off of the pymongo docs here since that's what we use. https://pymongo.readthedocs.io/en/stable/api/pymongo/collection.html#pymongo.collection.Collection.find_one_and_update I noticed that if you call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And yes the test |
||
return_document=pymongo.ReturnDocument.AFTER, | ||
) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the update can either be a pipeline or a single update, why do we need to only make it a pipeline under certain conditions and not just always like the current code? Is there a different bug that can happen by consistently using pipeline for all cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's nonstandard. I'd prefer to use the standard approach and deviate only when necessary.