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

feat: default aggregate lifespan configuration #548

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

yordis
Copy link
Contributor

@yordis yordis commented Jan 13, 2024

No description provided.

@yordis yordis changed the title feat default aggregate lifespan feat: default aggregate lifespan Jan 13, 2024
@yordis
Copy link
Contributor Author

yordis commented Jan 13, 2024

The feature is NOT finished (draft), but I would like to understand the expectations from the API perspective. Opening the conversation to the public!

Due to the multi-tenancy architecture, I like the idea of having a way to control the global configuration as well as routing.

When it comes to the routing, I am not sure if use :commanded OTP scope to configure things (it feels like an anti-pattern to me) or ask the user to pass the otp_app option and if that option is defined, then I try to read the config for the given Router module.

I am not sure, I opened to gather feedback about the direction of the feature.

@yordis yordis changed the title feat: default aggregate lifespan feat: default aggregate lifespan configuration Jan 13, 2024
@slashdotdash
Copy link
Member

slashdotdash commented Jan 13, 2024

The two changes we want to make are to modify the default aggregate lifespan behaviour defined by Commanded to not keep aggregate processes running indefinitely and to allow this default to be configured per Commanded Application.

We can do this by changing the Commanded.Aggregates.DefaultLifespan module to return an appropriate timeout value, such as a few hours, which would mean that by default aggregates are stopped after a period of inactivity. This is already implemented in the Aggregate process and handled automatically by the GenServer handle_* callback function timeout. Changing this default would apply to all users of Commanded who have not explicitely defined an aggregate lifespan via a router module.

Secondly, we want to make the default aggregate timeout configurable per Commanded Application. This configuration could be set either in the use macro, in configuration (such as config/config.exs), or at runtime via the init/1 callback function. Ideally we should support all three, which in practice means we need to support runtime configuration.

# Configured by `use` macro
defmodule MyApp.Application do
  use Commanded.Application,
    otp_app: :my_app,
    aggregate_default_lifespan: MyDefaultAggregateLifespan
end
# Configured by config file (config/config.exs)
config :my_app, MyApp.Application, aggregate_default_lifespan: MyDefaultAggregateLifespan
# Configured at runtime
defmodule MyApp.Application do
  use Commanded.Application, otp_app: :my_app

  def init(config) do
    config = Keyword.put(config, :aggregate_default_lifespan, MyDefaultAggregateLifespan)
    {:ok, config}
  end
end

@yordis
Copy link
Contributor Author

yordis commented Sep 28, 2024

The following,

use Commanded.Application,
  otp_app: :my_app,
  aggregate_default_lifespan: MyDefaultAggregateLifespan

That could be done using default_dispatch_opts.lifespan

use Commanded.Application,
  otp_app: :my_app,
  default_dispatch_opts: [lifespan: MyDefaultAggregateLifespan]

@yordis
Copy link
Contributor Author

yordis commented Sep 28, 2024

config :my_app, MyApp.Application, aggregate_default_lifespan: MyDefaultAggregateLifespan

This is done, fallback to config :commanded, AppName, aggregate_default_lifespan: MyDefaultAggregateLifespan as well

@yordis yordis force-pushed the feat-default-aggregate-lifespan branch 3 times, most recently from 5311eb1 to 72f9c6f Compare September 28, 2024 19:58
@yordis
Copy link
Contributor Author

yordis commented Sep 28, 2024

Runtime (init) should be handle by the same default_dispatch_opts as far as I can tell,

 def init(config) do
        config =
          Keyword.update(
            config,
            :default_dispatch_opts,
            [],
            &Keyword.put(&1, :lifespan, MyDefaultAggregateLifespan)
          )

        {:ok, config}
 end

No?!

@yordis
Copy link
Contributor Author

yordis commented Sep 28, 2024

I think that such init/1 is being called at compile time, as of today, I never in runtime thou.

Regardless, do we really need to add every possible combination or should we be pragmatic about it until somebody ask to add more things?

@yordis yordis marked this pull request as ready for review November 7, 2024 00:13
@yordis yordis force-pushed the feat-default-aggregate-lifespan branch from 72f9c6f to c6167da Compare November 7, 2024 16:20
@jwilger jwilger merged commit 2ff1263 into commanded:master Nov 7, 2024
5 checks passed
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.

4 participants