feat: SIGNAL-5504: add optional timer based work

[seungjinstord]

SIGNAL-5504

There's a need to have Buffy operate in a different flow than original.

Instead of changing the existing flow, a new behavior + using macro was created. Key differences:

It keeps the following functionality:
- Wait for a specified amount of time before
invoking the work function. If the function is called again before the time has
elapsed, it's a no-op.

Key differences:
- it will not be terminated once the timer is done, but kept alive
- internally, the existing timer behavior is done via state rather than handling {:error, {:already_started, pid}} output of GenServer.start_link.
- Under Horde, the state unfortunately doesn't get synced up automatically - that requires explicit tooling.
Therefore state will be "reset" to the initial state when process boots up. This is not a big issue as the initial state is to
set a throttled run of handle_throttle.
- it will be given the option (set by config) to trigger work repeatedly based on a empty inbox timeout interval,
that is based on GenServer's timeout feature.

How to Locally Test:

You need to clone this and in wms-service's mix.exs, use the local copy for dependency. You might need to also build this library (no need to build this library).
In deps, change the :buffy import statement to something like:

{:buffy, path: "/Users/[username]/path/to/buffy"},

(if you need to know the full path to buffy, do pwd in whereever you git cloned this repo)

Also, in wms-service, do rm -rf deps && rm -rf _build && mix deps.get for safe measure (might not need to do so), in the off chance that released buffy is lingering around.

Then, change the FulfillableStatusCheckBuffer:

defmodule WarehouseEvents.FulfillableStatusCheckBuffer do
  use Buffy.ThrottleAndTimed, #<-- here
    registry_module: Horde.Registry,
    registry_name: Warehouse.HordeRegistry,
    supervisor_module: Horde.DynamicSupervisor,
    supervisor_name: Warehouse.DistributedSupervisor,
    throttle: Application.get_env(:warehouse, :fulfillable_status_check_throttle),
    loop_interval: 2_000 # <-- 2 seconds 

  require Logger

To test/make the timed interval triggers run without an initial order creation (as in when application starts), do something along the lines of:

Although local test seems to show there's some code that triggers throttle()(see loom), we want to explicitly start a timed interval in the final implementation. We need to create a child spec for the application Supervisor (typically in application.ex) for a Task module, that does a one-off run to start however many instances of throttle/1 as necessary. That way, the default inbox timeout will run even without any triggers. Following code can be wrapped up in a module, but for testing you can do some variation of:

# application.ex
def start(_type, _args) do
  ...
  children = [
    ...
    # Start timed interval run of WarehouseEvents.FulfillableStatusCheckBuffer for all valid shippers
    {true,
     Supervisor.child_spec(
       {Task,
        fn ->
          building_id = "ac61643b-8eed-4aa4-a377-1e197a91733d"
          building = Warehouse.Repo.get_by(Warehouse.Facility.Building, id: building_id)
          user = Warehouse.Facility.User.system_user(building)

          %{entries: shippers} =
            Warehouse.Facility.list_shippers(%{"buiding_ids" => building_id, "page_size" => 1000}, user)

          for s <- shippers do
            WarehouseEvents.FulfillableStatusCheckBuffer.throttle({building, s.id})
          end
        end},
       id: FulfillableStatusCheckBufferInit,
       restart: :temporary
     )}
  ]
  ...

Loom going over the above changes needed for local testing, and showing the current / new behavior differences:

https://www.loom.com/share/e65917c74fe846529af4f2c226abfba2

[seungjinstord]

@btkostner this is mostly based off of the existing throttle code, but tweaked a bit due to the need to also introduce time-based repeated work on top of just trigger-invoked throttling. Please let me know if this is the route we can take, or if I'm completely off.

[doomspork]

I wonder if we should investigate a way for us to use Buffy.Throttle here and override the functions we need with this implementation's specifics to make iterating on the underlying Throttle features easier and reduce having to implement in multiple places 🤔

[kinson]

I'm not 100% sure that adding that layer of abstraction would help here. I'd be concerned with the readability of this module due to the fact that it is already somewhat complex and implements a macro.

Furthermore, these modules may share a similar name but they are fundamentally different in that Buffy.Throttle executes the task and stops the server immediately whereas Buffy.ThrottleAndTimed is a long-running process.

Maybe I don't have the same vision for how we could layer these two together though, so if you have something in mind I could definitely be persuaded!

[btkostner]

Note: high brain fog, might be talking jubberish.

Isn't this just a debounce instead of a throttle? https://lodash.com/docs/#debounce

[doomspork]

Just noticed this is feat!, since this is a net new behaviour is this a breaking change?

[seungjinstord]

@doomspork hm yeah existing behavior isn't touched; this is a net new. I will change it to feat - originally I thought this was a net new so it warrants a feat!

[kinson]

A couple minor things, but overall this is lookin' good 😎

I'll test it out locally with wms-service next!

[kinson]

TIL you could write a test like this 🤯

[seungjinstord]

I know haha I was like wait this works?

[kinson]

Tested this locally and it worked as expected 👨🏻‍🍳 👏🏻 - only suggestions are documentation/code readability related!

[kinson]

I feel like we could come up with a slightly better name for this to describe the use case but I am struggling to land on one. Maybe ThrottleWithTimeout, or ThrottleWithLoop? I think we can keep it as is for now and iterate on it later though unless we can come up with a better option without spending too much time on it.

[seungjinstord]

ThrottleWithLoopInterval?

[kinson]

@doomspork & @seungjinstord should this pr be into main or the code freeze branch? Because this is a library and not a service, merging into code freeze seems...unusual because a service would need to opt in to using these changes by updating the library version.

[seungjinstord]

@kinson yeah that branch got created a couple days ago so I thought it might be the way to go. main branch has a change that is not in the code freeze branch (adding jitter).

[kinson]

@seungjinstord should we change the base for this pr to main then? Of course, we should double check with @doomspork first, but I feel like that might make the most sense in this case.

[seungjinstord]

@kinson talked with @doomspork , and target branch has been updated to main.