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.

Sign up for GitHub

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

Jump to bottom

Use long-lived producer instance(s) #16

Closed
timmc-edx opened this issue Jul 29, 2022 · 2 comments
Closed

Use long-lived producer instance(s) #16

timmc-edx opened this issue Jul 29, 2022 · 2 comments
Assignees
@timmc-edx
Labels
event-bus Work related to the Event Bus.
Milestone
[Event Bus] Imple...

Comments

@timmc-edx
Copy link
Contributor

timmc-edx commented Jul 29, 2022 •
edited
Loading

event_bus.send instantiates a SerializingProducer every time it is called, but producers have a destructor that destroys their outbound queue immediately when they go out of scope. This means that even in a long-lived service, producers can't actually reliably send their messages to the brokers.

We can't keep just one producer and still use SerializingProducer, since those are locked to a single topic and signal. We might want to use Producer directly and handle the serializing explicitly, which would allow us to maintain a single Producer for all signals. This would entail copying and updating the code that is in SerializingProducer in our own custom class. Or, we could just keep a signal -> producer cache (maybe using @lru_cache) and just accept that there are multiple producers, but only a small constant number.

Code links:
@timmc-edx timmc-edx moved this to Todo in Arch-BOM Jul 29, 2022
@timmc-edx timmc-edx added event-bus Work related to the Event Bus. backlog Item is on a team's backlog or wish list. labels Jul 29, 2022
@timmc-edx timmc-edx changed the title Use a single long-lived producer instance Use long-lived producer instance(s) Aug 1, 2022
@robrap robrap changed the title Use long-lived producer instance(s) [WH] Use long-lived producer instance(s) Aug 2, 2022
@robrap robrap removed the backlog Item is on a team's backlog or wish list. label Aug 3, 2022
@robrap robrap moved this from Todo to In Progress in Arch-BOM Aug 3, 2022
@robrap robrap moved this from In Progress to Groomed in Arch-BOM Aug 4, 2022
@timmc-edx timmc-edx mentioned this issue Aug 9, 2022
Merged
9 tasks
@timmc-edx
Copy link
Contributor Author

I've updated the issue with new information. It's higher priority now.

@timmc-edx
Copy link
Contributor Author

#21 ensures that long-lived instances are used, although this approach of using @lru_cache may not be our permanent solution. Still worth moving away from SerializingProducer anyhow.

@whuang1202 whuang1202 moved this from Groomed to In Progress in Arch-BOM Aug 12, 2022
@timmc-edx timmc-edx assigned timmc-edx and unassigned whuang1202 Aug 22, 2022
@timmc-edx timmc-edx changed the title [WH] Use long-lived producer instance(s) Use long-lived producer instance(s) Aug 22, 2022
timmc-edx added a commit that referenced this issue Aug 23, 2022
@timmc-edx
feat: Never evict producers from cache
d211fee 
This should close out #16
by just embracing the use of multiple producers.

Also:

- Improve cache-clearing during tests by connecting to the setting_changed
  signal -- this allows knowledge of what's cached to be local to the
  module where the caching happens, rather than relying on each test module
  to know about all of the modules it depends on indirectly.
- Remove outdated comment about caching, and clarify other caching
  comments.
timmc-edx added a commit that referenced this issue Aug 23, 2022
@timmc-edx
fix: Never evict producers from cache
6c2a0b6 
This should close out #16
by just embracing the use of multiple producers.

Also:

- Improve cache-clearing during tests by connecting to the setting_changed
  signal -- this allows knowledge of what's cached to be local to the
  module where the caching happens, rather than relying on each test module
  to know about all of the modules it depends on indirectly.
- Remove outdated comment about caching, and clarify other caching
  comments.
@timmc-edx timmc-edx mentioned this issue Aug 23, 2022
Merged
4 tasks
timmc-edx added a commit that referenced this issue Aug 24, 2022
@timmc-edx
fix: Never evict producers from cache
fe987da 
This should close out #16
by just embracing the use of multiple producers.

Also:

- Improve cache-clearing during tests by connecting to the setting_changed
  signal -- this allows knowledge of what's cached to be local to the
  module where the caching happens, rather than relying on each test module
  to know about all of the modules it depends on indirectly.
- Remove outdated comment about caching, and clarify other caching
  comments.
timmc-edx added a commit that referenced this issue Aug 24, 2022
@timmc-edx
fix: Never evict producers from cache (#28)
b2b0405 
This should close out #16
by just embracing the use of multiple producers.

Also:

- Improve cache-clearing during tests by connecting to the setting_changed
  signal -- this allows knowledge of what's cached to be local to the
  module where the caching happens, rather than relying on each test module
  to know about all of the modules it depends on indirectly.
- Remove outdated comment about caching, and clarify other caching
  comments.
Repository owner moved this from In Progress to Done in Arch-BOM Aug 24, 2022
timmc-edx added a commit that referenced this issue Aug 26, 2022
@timmc-edx
feat!: Switch to a single Producer, wrapped in an API singleton
eb69287 
Purpose:

- Revisit #16 since I
  finally figured out a clean way to have a single producer.
- Reduce the burden on future code that will need to adjust how polling
  is done (#31) and
  maybe handle shutdown (#11)
- Prepare for configurable implementation loading, which will need a
  singleton and getter: openedx/openedx-events#87
- Get rid of the `sync` argument (which didn't fit the abstraction) and
  move it to a dedicated method.

Relying code should now call `get_producer().send(...)` rather than
`send_to_event_bus(...)`. The return value is an object that wraps a
`Producer` instance (not a `SerializingProducer`) and that handles the
serialization itself.

Serialization logic is moved to a cached `get_serializers(...)` that
expands upon the previous `get_serializer` function; it now returns a pair
of key and value serializers. This also acts as a patch point for mocking.

`send_to_event_bus` gets a shorter name (now it's just a `send` method)
and loses the `sync` keyword argument; there is instead now a
`pre_shutdown` method.
timmc-edx added a commit that referenced this issue Aug 26, 2022
@timmc-edx
feat!: Switch to a single Producer, wrapped in an API singleton
4fd2a37 
Purpose:

- Revisit #16 since I
  finally figured out a clean way to have a single producer.
- Reduce the burden on future code that will need to adjust how polling
  is done (#31) and
  maybe handle shutdown (#11)
- Prepare for configurable implementation loading, which will need a
  singleton and getter: openedx/openedx-events#87
- Get rid of the `sync` argument (which didn't fit the abstraction) and
  move it to a dedicated method.

Relying code should now call `get_producer().send(...)` rather than
`send_to_event_bus(...)`. The return value is an object that wraps a
`Producer` instance (not a `SerializingProducer`) and that handles the
serialization itself.

Serialization logic is moved to a cached `get_serializers(...)` that
expands upon the previous `get_serializer` function; it now returns a pair
of key and value serializers. This also acts as a patch point for mocking.

`send_to_event_bus` gets a shorter name (now it's just a `send` method)
and loses the `sync` keyword argument; there is instead now a
`pre_shutdown` method.
@timmc-edx timmc-edx mentioned this issue Aug 26, 2022
Merged
5 tasks
timmc-edx added a commit that referenced this issue Aug 26, 2022
@timmc-edx
feat!: Switch to a single Producer, wrapped in an API singleton
bbf0585 
Purpose:

- Revisit #16 since I
  finally figured out a clean way to have a single producer.
- Reduce the burden on future code that will need to adjust how polling
  is done (#31) and
  maybe handle shutdown (#11)
- Prepare for configurable implementation loading, which will need a
  singleton and getter: openedx/openedx-events#87
- Get rid of the `sync` argument (which didn't fit the abstraction) and
  move it to a dedicated method.

Relying code should now call `get_producer().send(...)` rather than
`send_to_event_bus(...)`. The return value is an object that wraps a
`Producer` instance (not a `SerializingProducer`) and that handles the
serialization itself.

Serialization logic is moved to a cached `get_serializers(...)` that
expands upon the previous `get_serializer` function; it now returns a pair
of key and value serializers. This also acts as a patch point for mocking.

`send_to_event_bus` gets a shorter name (now it's just a `send` method)
and loses the `sync` keyword argument; there is instead now a
`pre_shutdown` method.
timmc-edx added a commit that referenced this issue Aug 26, 2022
@timmc-edx
feat!: Switch to a single Producer, wrapped in an API singleton
a4b9e52 
Purpose:

- Revisit #16 since I
  finally figured out a clean way to have a single producer.
- Reduce the burden on future code that will need to adjust how polling
  is done (#31) and
  maybe handle shutdown (#11)
- Prepare for configurable implementation loading, which will need a
  singleton and getter: openedx/openedx-events#87
- Get rid of the `sync` argument (which didn't fit the abstraction) and
  move it to a dedicated method.

Relying code should now call `get_producer().send(...)` rather than
`send_to_event_bus(...)`. The return value is an object that wraps a
`Producer` instance (not a `SerializingProducer`) and that handles the
serialization itself.

Serialization logic is moved to a cached `get_serializers(...)` that
expands upon the previous `get_serializer` function; it now returns a pair
of key and value serializers. This also acts as a patch point for
mocking. I'd like to test the serializers themselves, but they want to
talk to a server.

`send_to_event_bus` gets a shorter name (now it's just a `send` method)
and loses the `sync` keyword argument; there is instead now a
`pre_shutdown` method.
timmc-edx added a commit that referenced this issue Aug 31, 2022
@timmc-edx
feat!: Switch to a single Producer, wrapped in an API singleton (#32)
fa7c4fd 
Purpose:

- Revisit #16 since I
  finally figured out a clean way to have a single producer.
- Reduce the burden on future code that will need to adjust how polling
  is done (#31) and
  maybe handle shutdown (#11)
- Prepare for configurable implementation loading, which will need a
  singleton and getter: openedx/openedx-events#87
- Get rid of the `sync` argument (which didn't fit the abstraction) and
  move it to a dedicated method.

Relying code should now call `get_producer().send(...)` rather than
`send_to_event_bus(...)`. The return value is an object that wraps a
`Producer` instance (not a `SerializingProducer`) and that handles the
serialization itself.

Serialization logic is moved to a cached `get_serializers(...)` that
expands upon the previous `get_serializer` function; it now returns a pair
of key and value serializers. This also acts as a patch point for
mocking. I'd like to test the serializers themselves, but they want to
talk to a server.

`send_to_event_bus` gets a shorter name (now it's just a `send` method)
and loses the `sync` keyword argument; there is instead now a
`prepare_for_shutdown` method.

Other refactoring:

- Cache `create_schema_registry_client` and rename to `get_...`
- Lift producer test data to be instance variables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@timmc-edx timmc-edx

Labels
event-bus Work related to the Event Bus.
Projects
None yet
Milestone
[Event Bus] Implement happy-path
Development

No branches or pull requests
3 participants
@robrap @timmc-edx @whuang1202