-
Notifications
You must be signed in to change notification settings - Fork 4
Add number of extracted, loaded, and failed resources in batch responses #693
Conversation
This field is a SerializerMethodField that fetches the progression for each resource from redis
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.
can you update river-schema.yml as well?
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.
IMO it's best to use a dedicated table for Progression
. having a jsonfield with a custom structure in the Batch table doesn't make sense in a relational db, especially if we want to make queries involving a progression counter by resource_id.
@tevariou what's your opinion ?
I agree it may be more practical to have a dedicated table for filtering |
Also, @tevariou, @simonvadee, do you know if there is an easy way to know the number of resources that have been updated compared to inserted? |
good question: there isn't. In |
@@ -11,6 +15,7 @@ class Meta: | |||
|
|||
class BatchSerializer(serializers.ModelSerializer): | |||
errors = ErrorSerializer(many=True, read_only=True) | |||
progressions = serializers.SerializerMethodField() |
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 we created a model for this field, maybe we need to do something like that https://www.django-rest-framework.org/api-guide/relations/#nested-relationships
But it adds complexity (and I know Valentin didn't like nested serializers).. maybe it would be better to create a new route
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.
Right, but I didn't know how to combine a SerializerMethodField
and model Serializers... Do you know how we could do that?
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.
Changed to stop using SerializerMethodField
9d1791c
to
b6981ee
Compare
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.
only small comments 💯
@@ -21,6 +42,40 @@ class Meta: | |||
"completed_at": {"allow_null": True}, | |||
} | |||
|
|||
def get_progressions(self, obj) -> List[Tuple[str, Dict]]: |
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.
I think this is not used anymore: you use ProgressionSerializer and ResourceForProgressionSerializer instead
progression_factory.create(batch=batch, resource=r1) | ||
progression_factory.create(batch=batch, resource=r2) |
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.
can you define extracted/loaded/failed here instead of directly in the factory ? easier to understand
progression_factory.create(batch=batch, resource=r1) | ||
progression_factory.create(batch=batch, resource=r2) |
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.
same
|
||
response_get = api_client.get(url) | ||
assert response_get.json()["canceled_at"] is not None | ||
print(response_get.json()["progressions"]) |
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.
remove
extracted = 100 | ||
loaded = 50 | ||
failed = None |
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.
IMO: should be removed
TODO: front
Fixes
Fixes #668 by @MiskoG
Description
progressions
field toBatch
Technical details
If the batch is running, the progression is fetched from redis. Otherwise, it's persisted and fetched from the DB
Definition of Done