feat: add support for google-cloud-pubsub closes #25 #30
Conversation
Mohsin-Ul-Islam
changed the title
|
@Mohsin-Ul-Islam Hi, thank you for your contribution =) Would you please also include test cases to cover the usage of the handler? |
|
@melvinkcx Hi, I have added a test for GCP pubsub handler. I couldn't find a mock library for google cloud pubsub python client, so I am testing it with google cloud pubsub emulator. More information on emulator can be found here: https://cloud.google.com/pubsub/docs/emulator In order to spin up and tear down docker container I am using testcontainers library to spin up our pubsub emulator. I am a beginner in open source, so any guidance would be appreciated. Thanks a lot! |
|
@Mohsin-Ul-Islam I took a look at your test cases. The approach you take (integration tests) will significantly inflate the dependencies required to run the tests and for development (Docker, etc). I would suggest you mock the google clients yourself, and verify that the methods ( |
|
The tests are failing, you might need to update the "Install dependencies" step of the github workflow |
|
Sure, I will mock the google cloud pubsub client. |
Signed-off-by: Mohsin-Ul-Islam <mohsinulislam180@gmail.com>
Mohsin-Ul-Islam
force-pushed
the
features/gcp_pubsub_support
branch
from
8f304b8
to
0aafbd7
Compare
|
Hi, @Mohsin-Ul-Islam, it seems like the test is now fixed. Would you please confirm if the PR is ready for review? =) |
|
@melvinkcx yes the PR is ready for review. Thanks! |
There was a problem hiding this comment.
Hi @Mohsin-Ul-Islam, thanks for contributing. Please check the comments I added. Also, please update the README to document the usage of your handler =)
fastapi_events/handlers/gcp.py
Outdated
| return json.dumps(event, default=str) | ||
|
|
||
|
|
||
| class GoogleCloudPubSubHandler(BaseEventHandler): |
There was a problem hiding this comment.
@Mohsin-Ul-Islam I believe there will be needs to forward events to multiple topics depending on the event name, as some folks using AWS SNS (the pub/sub offering by AWS) have indicated.
What about renaming GoogleCloudPubSubHandler to GoogleCloudSimplePubSubHandler to allow room in the future to implement a GCP pub/sub handler with more advanced routing capabilities?
I think it'd be better if we rename it to something such as GoogleCloudSimplePubSubHandler to indicate that the proces
fastapi_events/handlers/gcp.py
Outdated
| # Publish messages as soon as there are max_messages | ||
| # or 1 second is passed | ||
| self._batch_settings = pubsub_v1.types.BatchSettings( | ||
| max_messages=self._max_batch_size, max_latency=1 |
There was a problem hiding this comment.
We should allow the users to customise their BatchSettings:
This is a suggestion:
def __init__(
self,
project_id: str,
topic_id: str,
max_batch_size: int = 1000,
batch_settings_kwargs: Dict[str, Any] = None,
serializer: Callable[[Event], str] = None,
) -> None:
...
self._batch_settings = pubsub_v1.types.BatchSettings(
max_messages=self._max_batch_size, **batch_settings_kwargs
)
There was a problem hiding this comment.
Hi, @melvinkcx, thank you so much for the review. The changes requested makes perfect sense. I will update the PR.
…s to underlying pubsub client
No description provided.