Write reminders in multiple partitions by artursouza · Pull Request #3297 · dapr/dapr

artursouza · 2021-06-16T06:55:36Z

Description

Split actor reminders storage into multiple keys.

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #2889

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Created/updated tests
Unit tests passing
End-to-end tests passing
Extended the documentation / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]
Specification has been updated / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]
Provided sample for the feature / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]

codecov · 2021-06-16T08:22:43Z

Codecov Report

Merging #3297 (0ad7505) into master (032f108) will increase coverage by 0.74%.
The diff coverage is 70.54%.

@@            Coverage Diff             @@
##           master    #3297      +/-   ##
==========================================
+ Coverage   58.50%   59.25%   +0.74%     
==========================================
  Files          84       96      +12     
  Lines        7611     8482     +871     
==========================================
+ Hits         4453     5026     +573     
- Misses       2848     3101     +253     
- Partials      310      355      +45

Impacted Files	Coverage Δ
cmd/placement/config.go	`27.02% <ø> (ø)`
cmd/placement/main.go	`0.00% <ø> (ø)`
pkg/components/bindings/registry.go	`100.00% <ø> (ø)`
pkg/components/kubernetes_loader.go	`50.00% <ø> (ø)`
pkg/components/middleware/http/registry.go	`100.00% <ø> (ø)`
pkg/components/nameresolution/registry.go	`100.00% <ø> (ø)`
pkg/components/pubsub/registry.go	`100.00% <ø> (ø)`
pkg/components/secretstores/registry.go	`100.00% <ø> (ø)`
pkg/components/standalone_loader.go	`61.81% <ø> (ø)`
pkg/credentials/certchain.go	`0.00% <ø> (ø)`
... and 99 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6bb566f...0ad7505. Read the comment docs.

berndverst

LGTM so far.

artursouza · 2021-06-18T07:28:26Z

LGTM so far.

There is a long way to go to make this prod ready. I still need to:

Enable the partition write and read path.
Make the partition configurable.
Implement partition config change strategy
Write E2E test with happy path.
Write E2E test with partition config change.

artursouza · 2021-06-21T02:50:42Z

/ok-to-test

dapr-bot · 2021-06-21T02:51:28Z

Found the available test cluster - dapr-aks-e2e-08 for linux. Please check the build status.

dapr-bot · 2021-06-21T02:52:01Z

Found the available test cluster - dapr-aks-e2e-05 for windows. Please check the build status.

dapr-bot · 2021-06-21T03:36:12Z

End-to-end tests failed on linux. Please check the build logs

artursouza · 2021-06-21T04:06:42Z

/ok-to-test

dapr-bot · 2021-06-21T04:07:28Z

Found the available test cluster - dapr-aks-e2e-07 for linux. Please check the build status.

dapr-bot · 2021-06-21T04:07:55Z

Found the available test cluster - dapr-aks-e2e-06 for windows. Please check the build status.

dapr-bot · 2021-06-21T04:09:21Z

End-to-end tests failed on windows. Please check the build logs

dapr-bot · 2021-06-21T05:04:40Z

End-to-end tests failed on linux. Please check the build logs

dapr-bot · 2021-06-21T05:19:43Z

End-to-end tests failed on windows. Please check the build logs

artursouza · 2021-07-06T00:11:10Z

/ok-to-test

artursouza · 2021-07-06T00:11:17Z

/ok-to-perf

dapr-bot · 2021-07-06T00:11:52Z

Found the available test cluster - dapr-aks-e2e-08 for linux. Please check the build status.

dapr-bot · 2021-07-06T00:12:01Z

Found the available test cluster - dapr-aks-perf-01. Please wait until test is done.

dapr-bot · 2021-07-06T00:12:36Z

Found the available test cluster - dapr-aks-e2e-05 for windows. Please check the build status.

dapr-bot · 2021-07-06T00:17:39Z

Dapr Perf tests failed.

dapr-bot · 2021-07-06T00:48:53Z

Congrats! All end-to-end tests have passed on linux. Thanks for your contribution!

dapr-bot · 2021-07-06T01:19:06Z

End-to-end tests failed on windows. Please check the build logs

artursouza · 2021-07-06T03:07:38Z

/ok-to-test

artursouza · 2021-07-06T03:07:45Z

/ok-to-perf

dapr-bot · 2021-07-06T03:08:21Z

Found the available test cluster - dapr-aks-e2e-07 for linux. Please check the build status.

dapr-bot · 2021-07-06T03:08:40Z

Found the available test cluster - dapr-aks-perf-01. Please wait until test is done.

dapr-bot · 2021-07-06T03:09:13Z

Found the available test cluster - dapr-aks-e2e-06 for windows. Please check the build status.

dapr-bot · 2021-07-06T03:14:16Z

Dapr Perf tests failed.

Use batch and trx APIs to reminder partitions.

…dway.

artursouza · 2021-07-09T04:31:27Z

/ok-to-test

artursouza · 2021-07-09T04:31:38Z

/ok-to-perf

dapr-bot · 2021-07-09T04:32:04Z

Found the available test cluster - dapr-aks-e2e-08 for linux. Please check the build status.

dapr-bot · 2021-07-09T04:32:39Z

Found the available test cluster - dapr-aks-e2e-06 for windows. Please check the build status.

dapr-bot · 2021-07-09T05:08:55Z

Congrats! All end-to-end tests have passed on linux. Thanks for your contribution!

dapr-bot · 2021-07-09T05:40:37Z

End-to-end tests failed on windows. Please check the build logs

markrussinovich · 2021-07-09T20:58:00Z

@@ -831,14 +850,102 @@ func (a *actorsRuntime) getReminder(req *CreateReminderRequest) (*Reminder, bool
 	a.remindersLock.RUnlock()

 	for _, r := range reminders {


This for loop might have scaling problems in the future. Suggest considering indices for pre-filtering.

getReminder() is used only in the CreateReminder() method. Right now, the bottleneck is more in the IO needed to retrieve all reminders for a given ActorType. After we improve throughput from the IO constraints, we will look into in-memory enhancements in this particular path.

On the other hand, I found similar loops in the code where we could improve by indexing per ActorType/ActorId/ReminderName combination.

Tracking it in #3412

halspang

LGTM, just a nitpick.

yaron2 · 2021-07-12T15:48:43Z

+	}
+
+	if actorMetadata.RemindersMetadata.PartitionCount > a.config.RemindersStoragePartitions {
+		log.Warnf("cannot decrease number of partitions for reminders of actor type %s", actorType)


Agreed with @artursouza.

yaron2

LGTM

artursouza requested a review from berndverst June 16, 2021 06:55

artursouza marked this pull request as draft June 16, 2021 06:55

berndverst reviewed Jun 16, 2021

View reviewed changes

artursouza changed the base branch from release-1.2 to master June 21, 2021 02:50

artursouza added 10 commits July 8, 2021 21:31

Write reminders in partitions but handles legacy case only.

b96bf84

Handles partitioning of reminders data + E2E test

0c1bfe8

Move reminder partition test to another test file.

1712e81

Adds feature flag for Actor Type Metadata.

beabae4

Adds unit test to create reminders with partition.

e481aff

Add unit test to delete reminder with partition.

ffd7ebc

Parallel R/W for reminder partitions.

80135b3

Use batch and trx APIs to reminder partitions.

Fixing E2E test.

dbf981c

Fix conflict.

5d7d0c1

Add more actors to reminder partition e2e. Change partition number mi…

af8ff84

…dway.

markrussinovich reviewed Jul 9, 2021

View reviewed changes

artursouza mentioned this pull request Jul 9, 2021

Index actor reminders in memory #3412

Open

halspang previously approved these changes Jul 9, 2021

View reviewed changes

Comment thread pkg/actors/actors.go Outdated

Merge branch 'master' into split_reminder_db

36c0832

yaron2 reviewed Jul 12, 2021

View reviewed changes

improving some comments.

75bda4e

artursouza dismissed halspang’s stale review via 75bda4e July 12, 2021 17:54

yaron2 approved these changes Jul 12, 2021

View reviewed changes

artursouza added the automerge Allows DaprBot to automerge and update PR if all approvals are in place label Jul 12, 2021

Merge branch 'master' into split_reminder_db

0ad7505

dapr-bot merged commit 22bb68b into dapr:master Jul 12, 2021

This was referenced Jul 12, 2021

Configurable actor reminder storage partitions dapr/dotnet-sdk#712

Merged

Configurable actor reminder storage patitions dapr/python-sdk#236

Merged

berndverst mentioned this pull request May 10, 2022

Add support for Actor Reminder Partitioning dapr/go-sdk#284

Open

		@@ -831,14 +850,102 @@ func (a actorsRuntime) getReminder(req CreateReminderRequest) (*Reminder, bool
		a.remindersLock.RUnlock()

		for _, r := range reminders {

Uh oh!

Conversation

artursouza commented Jun 16, 2021

Description

Issue reference

Checklist

Uh oh!

codecov Bot commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

berndverst left a comment

Choose a reason for hiding this comment

Uh oh!

artursouza commented Jun 18, 2021

Uh oh!

artursouza commented Jun 21, 2021

Uh oh!

dapr-bot commented Jun 21, 2021

Uh oh!

dapr-bot commented Jun 21, 2021

Uh oh!

dapr-bot commented Jun 21, 2021

Uh oh!

artursouza commented Jun 21, 2021

Uh oh!

dapr-bot commented Jun 21, 2021

Uh oh!

dapr-bot commented Jun 21, 2021

Uh oh!

dapr-bot commented Jun 21, 2021

Uh oh!

dapr-bot commented Jun 21, 2021

Uh oh!

dapr-bot commented Jun 21, 2021

Uh oh!

artursouza commented Jul 6, 2021

Uh oh!

artursouza commented Jul 6, 2021

Uh oh!

dapr-bot commented Jul 6, 2021

Uh oh!

dapr-bot commented Jul 6, 2021

Uh oh!

dapr-bot commented Jul 6, 2021

Uh oh!

dapr-bot commented Jul 6, 2021

Uh oh!

dapr-bot commented Jul 6, 2021

Uh oh!

dapr-bot commented Jul 6, 2021

Uh oh!

artursouza commented Jul 6, 2021

Uh oh!

artursouza commented Jul 6, 2021

Uh oh!

dapr-bot commented Jul 6, 2021

Uh oh!

dapr-bot commented Jul 6, 2021

Uh oh!

dapr-bot commented Jul 6, 2021

Uh oh!

dapr-bot commented Jul 6, 2021

Uh oh!

artursouza commented Jul 9, 2021

Uh oh!

artursouza commented Jul 9, 2021

Uh oh!

dapr-bot commented Jul 9, 2021

Uh oh!

dapr-bot commented Jul 9, 2021

Uh oh!

dapr-bot commented Jul 9, 2021

Uh oh!

dapr-bot commented Jul 9, 2021

Uh oh!

markrussinovich Jul 9, 2021

Choose a reason for hiding this comment

Uh oh!

artursouza Jul 9, 2021

codecov Bot commented Jun 16, 2021 •

edited

Loading