Skip to content

chore: improvements around dynamic scaling of sharded daemon processes - #1437

Open
johanandren wants to merge 2 commits into
mainfrom
wip-dynamic-scaling-improvements
Open

chore: improvements around dynamic scaling of sharded daemon processes#1437
johanandren wants to merge 2 commits into
mainfrom
wip-dynamic-scaling-improvements

Conversation

@johanandren

Copy link
Copy Markdown
Contributor

ShardedDaemonProcess keeps the current number of processes in distributed data, and the (initial)NumberOfInstances argument is only used on first start. Several gRPC projection samples and the internal ReplicationImpl computed sliceRanges outside the behavior factory, sized by the configured number. If ddata held a different (e.g. previously rescaled) count, processes with a processNumber outside the configured range would crash with IndexOutOfBoundsException on every keep-alive tick, silently degrading the projection to only the lowest-numbered instances doing useful work.

This PR migrates all affected sites to the pattern already used by the r2dbc/dynamodb doc examples: compute sliceRanges inside the factory from daemonContext.totalProcesses, and use initWithContext so it picks up rescale events.

The samples don't do rescaling themselves but could end up being the starting point for users who later would do rescaling.

Comment thread docs/src/main/paradox/running.md Outdated
daemonContext -> {
var sliceRanges =
EventSourcedProvider.sliceRanges(
system, R2dbcReadJournal.Identifier(), daemonContext.totalProcesses());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why do we use EventSourcedProvider here and in some other places

Persistence(system).sliceRanges(context.totalProcesses)

would be easier to always use Persistence.sliceRanges?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agree, strange

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

21 places across projections samples and specs (!!)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What it adds is looking up the read journal for that specific plugin/config, so that the actual journal could have some other slicing scheme.

But in practice it is always the same, so should we always use the simpler one? In that case, why does the EventSourcedProvider#sliceRanges even exist?

PR adding the method didn't shed any light on it #609

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ok, I would guess that the guy thought it would be nice to stay within projections api surface and not reach out to persistence. It's typically used together with

        EventSourcedProvider.eventsBySlices(
            system,
            R2dbcReadJournal.Identifier(),
            PRODUCER_ENTITY_TYPE,
            sliceRange.first(),
            sliceRange.second());

Let's leave it as is, using EventSourcedProvider#sliceRanges

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Might look better to move the whole slice range stuff into the projection method, and just pass the totalProcesses to that method, but not very important.


ShardedDaemonProcess.get(system)
.init(
.initWithContext(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we want to promote initWithContext in all places, even if scaling isn't used/needed? the signature looks more complex than init?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm, good point, I went for all the places to make sure anything users could copy paste is safe to evolve/scale, but maybe that is overdoing it.

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.

2 participants