Skip to content
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

Publish kafka message when resource is created or modified #2733

Merged
merged 28 commits into from May 17, 2022

Conversation

quaxsze
Copy link
Collaborator

@quaxsze quaxsze commented May 4, 2022

@quaxsze quaxsze changed the title Publish kafka message when resource is created or mordified Publish kafka message when resource is created or modified May 4, 2022
@quaxsze quaxsze requested a review from maudetes May 5, 2022 08:09
Copy link
Contributor

@maudetes maudetes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌟 Nice PR!

I don't think I could make the kafka messages being produced yet.

If I'm correct, on update_resource API call, update_resource method is not called, thus no signal on_resource_updated would be sent.

udata/core/dataset/events.py Outdated Show resolved Hide resolved
udata/core/dataset/models.py Outdated Show resolved Hide resolved
@@ -682,7 +682,9 @@ def add_resource(self, resource):
}
})
self.reload()
self.on_resource_added.send(document=self, resource_id=resource.id)
print('BEFORE SEND')
self.on_resource_added.send(self.__class__, document=self, resource_id=resource.id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to explicitly send self.__class__? It doesn't seem used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first parameter of the send method is sender, and we use to send the sender as the first param elsewhere in the software.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use the params we want, having document and resource_id only?

udata/core/dataset/events.py Outdated Show resolved Hide resolved
udata/settings.py Outdated Show resolved Hide resolved
quaxsze and others added 2 commits May 9, 2022 14:19
Co-authored-by: maudetes <maudet.estelle@gmail.com>
@quaxsze quaxsze requested a review from maudetes May 9, 2022 13:02
udata/core/dataset/events.py Outdated Show resolved Hide resolved
udata/core/dataset/events.py Show resolved Hide resolved
from udata.event.producer import produce


def serialize_resource_for_event(resource):
Copy link
Contributor

@maudetes maudetes May 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move dataset serialization in events as well? (same question for reuses and orgas) 🤔

@quaxsze quaxsze requested a review from maudetes May 10, 2022 11:22
CHANGELOG.md Outdated Show resolved Hide resolved
udata/core/dataset/api.py Show resolved Hide resolved
udata/core/dataset/api.py Show resolved Hide resolved
@@ -682,7 +682,9 @@ def add_resource(self, resource):
}
})
self.reload()
self.on_resource_added.send(document=self, resource_id=resource.id)
print('BEFORE SEND')
self.on_resource_added.send(self.__class__, document=self, resource_id=resource.id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use the params we want, having document and resource_id only?

udata/event/__init__.py Outdated Show resolved Hide resolved
udata/settings.py Outdated Show resolved Hide resolved
udata/core/dataset/events.py Show resolved Hide resolved
quaxsze and others added 3 commits May 10, 2022 16:45
Co-authored-by: maudetes <maudet.estelle@gmail.com>
@quaxsze quaxsze requested a review from maudetes May 10, 2022 14:49
udata/core/dataset/events.py Show resolved Hide resolved
udata/core/dataset/api.py Outdated Show resolved Hide resolved
udata/core/dataset/events.py Show resolved Hide resolved
udata/core/dataset/events.py Outdated Show resolved Hide resolved
Copy link
Contributor

@maudetes maudetes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost good I think 😊

udata/core/dataset/events.py Outdated Show resolved Hide resolved
udata/core/dataset/tasks.py Outdated Show resolved Hide resolved
@quaxsze quaxsze requested a review from maudetes May 17, 2022 09:26
@@ -52,8 +52,7 @@ def purge_datasets(self):
# Remove each dataset's resource's file
storage = storages.resources
for resource in dataset.resources:
if resource.fs_filename is not None:
storage.delete(resource.fs_filename)
dataset.remove_resource(resource)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iterating on a list while removing its elem seem like a bad idea (skip some iteration) sadly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put back the logic:

if resource.fs_filename is not None:
    storage.delete(resource.fs_filename)
Dataset.on_resource_removed.send(Dataset, document=dataset, resource_id=resource.id)

without the dataset.remove_resource, as the resources will be deleted with the dataset deletion and the important is to delete the file in the storage and to emit the signal.

@quaxsze quaxsze requested a review from maudetes May 17, 2022 12:16
Copy link
Contributor

@maudetes maudetes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@quaxsze quaxsze merged commit c1a3928 into master May 17, 2022
@quaxsze quaxsze deleted the producerResourceEvents branch May 17, 2022 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Production de messages de création/modification de ressources dans udata
2 participants