Skip to content

Conversation

@filipecabaco
Copy link
Member

What kind of change does this PR introduce?

scope postgres cdc syn registry

Comment on lines 73 to +74
users_scope_shards = Env.get_integer("USERS_SCOPE_SHARDS", 5)
postgres_cdc_scope_shards = Env.get_integer("POSTGRES_CDC_SCOPE_SHARDS", 5)
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we should document this as part of the README?

@@ -0,0 +1,21 @@
defmodule Realtime.Syn.PostgresCdc do
Copy link
Member

Choose a reason for hiding this comment

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

:nitpick: Should this be scoped (no pun intended) like this Extensions.PostgresCdcRls.Syn instead? No strong opinion tbh as this is highly subjective.

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to do something similar for Connect so probably will keep them all together so we can abstract more syn stuff

@moduledoc """
Scope for the PostgresCdc module.
"""
require Logger
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this Logger?

"""
@spec scope(String.t()) :: atom()
def scope(tenant_id) do
shards = Application.get_env(:realtime, :postgres_cdc_scope_shards)
Copy link
Member

Choose a reason for hiding this comment

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

Should we make this a fetch_env! and fail badly if not defined as nil here will not go well I think?

end

def scopes() do
shards = Application.get_env(:realtime, :postgres_cdc_scope_shards)
Copy link
Member

Choose a reason for hiding this comment

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

same here

scope = Atom.to_string(scope)

case scope do
"realtime_postgres_cdc_" <> _ ->
Copy link
Member

Choose a reason for hiding this comment

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

:nitpick: What if we keep this as part of the Syn module you created and not expose the prefix here?

@postgres_cdc_scope_prefix  Realtime.Syn.PostgresCdc.scope_prefix

case scope do
      @postgres_cdc_scope_prefix <> _ ->
...


def on_process_unregistered(scope, name, pid, _meta, reason) do
case Atom.to_string(scope) do
"realtime_postgres_cdc_" <> _ = scope ->
Copy link
Member

@edgurgel edgurgel Oct 27, 2025

Choose a reason for hiding this comment

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

same here

@coveralls
Copy link

coveralls commented Oct 27, 2025

Coverage Status

coverage: 86.747% (+0.03%) from 86.713%
when pulling 205963a on fix/partition-cdc-syn-scopes
into dec8f0c on main.

Copy link
Member

@edgurgel edgurgel left a comment

Choose a reason for hiding this comment

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

Added some comments. LGTM

@filipecabaco filipecabaco force-pushed the fix/partition-cdc-syn-scopes branch from 012181d to 205963a Compare October 28, 2025 14:35
Comment on lines +195 to +196
| POSTGRES_CDC_SCOPE_SHARDS | number | Number of dynamic supervisor partitions used by the Postgres CDC extension. Defaults to 5. |
| USERS_SCOPE_SHARDS | number | Number of dynamic supervisor partitions used by the Users extension. Defaults to 5. |
Copy link
Member

Choose a reason for hiding this comment

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

🤔 I think the description is wrong here

@filipecabaco filipecabaco merged commit 4653393 into main Oct 29, 2025
5 of 6 checks passed
@filipecabaco filipecabaco deleted the fix/partition-cdc-syn-scopes branch October 29, 2025 00:42
@kiwicopple
Copy link
Member

🎉 This PR is included in version 2.57.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants