[Fixes #13493] Track importer's celery task results using ExecutionRequest model #13494
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #13494 +/- ##
==========================================
+ Coverage 73.71% 73.79% +0.08%
==========================================
Files 921 921
Lines 54192 54277 +85
Branches 6168 6180 +12
==========================================
+ Hits 39945 40055 +110
+ Misses 12644 12627 -17
+ Partials 1603 1595 -8 🚀 New features to boost your workflow:
|
Gpetrak
force-pushed
the
ISSUE_13493
branch
from
e207fd7 to
75c6fec
Compare
| from geonode.resource.manager import resource_manager | ||
| from geonode.resource.models import ExecutionRequest | ||
| from geonode.upload.api.exceptions import ImportException | ||
| from geonode.upload.celery_tasks import ErrorBaseTaskClass, import_orchestrator |
There was a problem hiding this comment.
is the ErrorBaseTaskClass still needed?
| from osgeo import ogr | ||
| from django_celery_results.models import TaskResult | ||
|
|
||
| from geonode.upload.handlers.gpkg.tasks import SingleMessageErrorHandler |
There was a problem hiding this comment.
is SingleMessageErrorHandler still needed? if not we can remove it
| Called when the task fails. | ||
| Updates the ExecutionRequest.tasks dict and delegates logging/error handling to evaluate_error. | ||
| """ | ||
| task_name = self.name |
There was a problem hiding this comment.
is there a way to keep this part in common in a private function of the Task? Since i see that is repeted, just the state of the set_task_status changes
task_name = self.name
execution_id = args[0]
layer_key = find_key_recursively(kwargs, "layer_key")
self.set_task_status(task_name, execution_id, layer_key, "FAILED")| """ | ||
| Set the task status for the on_success and on_failure celery methods | ||
| """ | ||
| _exec = orchestrator.get_execution_object(execution_id) |
There was a problem hiding this comment.
@Gpetrak as a note for the future, we should evaluate to have a cache for this function.
| layer_names, _, _ = self.handler().import_resource(self.files, execution_id, **kwargs) | ||
|
|
||
| # Register the tasks_status with the created key alternates | ||
| orchestrator.register_task_status(execution_id, layer_names, task_name, status="RUNNING") |
There was a problem hiding this comment.
i would suggest to keep this in a private function of the Datastore object. In case we need to make changes to the function at least will be in a single place
| ) | ||
|
|
||
| # We create the layer key through which the layer is stored in the tasks schema | ||
| kwargs["layer_key"] = create_layer_key(layer_name, str(execution_id)).lower() |
There was a problem hiding this comment.
is possible to keep the .lower() in the create_layer_key method instead of here? Or in some other cases should be kept as it is? having the .lower() in the method will keep consistency
This PR was created according to this issue: #13493
Checklist
For all pull requests:
The following are required only for core and extension modules (they are welcomed, but not required, for contrib modules):
Submitting the PR does not require you to check all items, but by the time it gets merged, they should be either satisfied or inapplicable.