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

Actor Reminders: Default JSON serialization. #7548

Merged

Conversation

JoshVanL
Copy link
Contributor

To support downgrades to 1.12 from 1.13, this PR changes the reminder serialization storage format back to JSON by default. This means a 1.12 actor reminder client can read reminders written by 1.13 actors.

1.13 will continue to understand both JSON and protobuf. Protobuf serialization can be enabled with the ActorReminderStorageProtobuf feature gate. The actor "API Level" has been changed back to 10.

Adds test to ensure the default serialization is JSON.

To support downgrades to 1.12 from 1.13, this PR changes the reminder
serialization storage format back to JSON by default. This means a 1.12
actor reminder client can read reminders written by 1.13 actors.

1.13 will continue to understand both JSON and protobuf. Protobuf
serialization can be enabled with the `ActorReminderStorageProtobuf`
feature gate. The actor "API Level" has been changed back to 10.

Adds test to ensure the default serialization is JSON.

Signed-off-by: joshvanl <me@joshvanl.dev>
@JoshVanL JoshVanL marked this pull request as ready for review February 19, 2024 18:20
@JoshVanL JoshVanL requested review from a team as code owners February 19, 2024 18:20
@JoshVanL JoshVanL added this to the v1.13 milestone Feb 19, 2024
Copy link

codecov bot commented Feb 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (05868fe) 62.14% compared to head (24c0586) 62.10%.

Additional details and impacted files
@@               Coverage Diff                @@
##           release-1.13    #7548      +/-   ##
================================================
- Coverage         62.14%   62.10%   -0.04%     
================================================
  Files               245      245              
  Lines             22389    22388       -1     
================================================
- Hits              13913    13905       -8     
+ Misses             7320     7319       -1     
- Partials           1156     1164       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@ItalyPaleAle ItalyPaleAle left a comment

Choose a reason for hiding this comment

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

This need not be a configuration option, because no one (eventually) will want to have JSON.

To keep 1.13 to use JSON by default you simply just need to change this line:

Change it to 10 and you're done :)

@JoshVanL
Copy link
Contributor Author

@ItalyPaleAle Helm values currently aren't testable so is safer to do in code imo. It also doesn't cover placement in self-hosted mode.

@ItalyPaleAle
Copy link
Contributor

@ItalyPaleAle Helm values currently aren't testable so is safer to do in code imo. It also doesn't cover placement in self-hosted mode.

This is not what was discussed in the call according to @mukundansundar ?

I really do not want a configuration option in the CRD as it will be there forever. The entire point of using API version in placement was so we could migrate more quickly, but adding a Configuration CRD option is a perennial choice.

For self-hosted we need to update the Docker Compose file examples. I would not be too concerned about self hosted as actor usage there is likely a tiny fraction.

@JoshVanL
Copy link
Contributor Author

The protobuf serialization is opt-in through a feature gate rather than a dedicated config CRD field, and expected to be removed in 1.14 where protobuf is the only supported option.

@JoshVanL JoshVanL force-pushed the actors-default-serialization-to-json branch from 62cff88 to 085d552 Compare February 20, 2024 14:14
level

Signed-off-by: joshvanl <me@joshvanl.dev>
@JoshVanL JoshVanL force-pushed the actors-default-serialization-to-json branch from 085d552 to fd62a24 Compare February 20, 2024 14:15
@JoshVanL JoshVanL mentioned this pull request Feb 20, 2024
@JoshVanL JoshVanL marked this pull request as draft February 21, 2024 11:46
Signed-off-by: joshvanl <me@joshvanl.dev>
@JoshVanL JoshVanL marked this pull request as ready for review February 21, 2024 13:07
@yaron2 yaron2 merged commit 5ecc595 into dapr:release-1.13 Feb 21, 2024
31 of 32 checks passed
JoshVanL added a commit to JoshVanL/dapr that referenced this pull request Mar 6, 2024
* Actor Reminders: Default JSON serialization.

To support downgrades to 1.12 from 1.13, this PR changes the reminder
serialization storage format back to JSON by default. This means a 1.12
actor reminder client can read reminders written by 1.13 actors.

1.13 will continue to understand both JSON and protobuf. Protobuf
serialization can be enabled with the `ActorReminderStorageProtobuf`
feature gate. The actor "API Level" has been changed back to 10.

Adds test to ensure the default serialization is JSON.

Signed-off-by: joshvanl <me@joshvanl.dev>

* Remove ActorReminderStorageProtobuf feature gate in favour of using API
level

Signed-off-by: joshvanl <me@joshvanl.dev>

* Fix api level tests

Signed-off-by: joshvanl <me@joshvanl.dev>

---------

Signed-off-by: joshvanl <me@joshvanl.dev>
Co-authored-by: Yaron Schneider <schneider.yaron@live.com>
yaron2 added a commit that referenced this pull request Mar 6, 2024
* Make injector resilient to sentry unavailability (#7507)

* make injector resilient to sentry unavailability

Signed-off-by: yaron2 <schneider.yaron@live.com>

* remove redundant line

Signed-off-by: yaron2 <schneider.yaron@live.com>

---------

Signed-off-by: yaron2 <schneider.yaron@live.com>

* sentry retry up to 30s (#7508)

Signed-off-by: yaron2 <schneider.yaron@live.com>

* Update contrib to 1.13.0-rc.3 (#7509)

* update contrib to 1.13.0-rc.2

Signed-off-by: yaron2 <schneider.yaron@live.com>

* update to rc.3

Signed-off-by: yaron2 <schneider.yaron@live.com>

---------

Signed-off-by: yaron2 <schneider.yaron@live.com>

* Injector: add option to add `DAPR_HOST_IP` env var to daprd (#7511)

The `DAPR_HOST_IP` env var is used in various places in Dapr for a sidecar to know its own IP address, for example for service invocation or actor invocation.

When using the Dapr injector to add the daprd container, we can use the downstream APIs to add the `DAPR_HOST_IP` env var based on data from the controller

This option can be enabled by setting the Helm option `dapr_sidecar_injector.enableK8sDownwardAPIs=true`

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

* Fix state encryption regression + add integration test (#7517)

* fix state encryption regression + add intg test

Signed-off-by: yaron2 <schneider.yaron@live.com>

* linter

Signed-off-by: yaron2 <schneider.yaron@live.com>

* review feedback

Signed-off-by: yaron2 <schneider.yaron@live.com>

* linter

Signed-off-by: yaron2 <schneider.yaron@live.com>

* just use sha256, remove nolint:gosec

Signed-off-by: yaron2 <schneider.yaron@live.com>

---------

Signed-off-by: yaron2 <schneider.yaron@live.com>

* Test Integration: speed up tests 10% (#7528)

* Test Integration: speed up tests 10%

Speed up integration tests by changing poll intervals
`100*time.Millisecond` to `10*time.Millisecond`.

~4.50m to ~4.20m

Signed-off-by: joshvanl <me@joshvanl.dev>

* Wait for operator healthz before exiting to ensure no exit error

Signed-off-by: joshvanl <me@joshvanl.dev>

---------

Signed-off-by: joshvanl <me@joshvanl.dev>

* Revert selfhosted disk loader to not respect namespace (#7527)

* Revert selfhosted disk loader to not respect namespace

Revert the selfhosted component disk loader to not respect the
namespace. This will allow the selfhosted disk loader to load components
from any namespace from file.

Fixes #7523

Signed-off-by: joshvanl <me@joshvanl.dev>

* Revert component disk loader behaviour to respect component namespace
when NAMESPACE env var is set.

Signed-off-by: joshvanl <me@joshvanl.dev>

---------

Signed-off-by: joshvanl <me@joshvanl.dev>

* [1.13] Add warning that Dapr state store encryption could lead to catastrophic failures (#7524)

* [1.13] Add warning that Dapr state store encryption could lead to catastrophic failures

If the same key is used to encrypt more than 2^32 values (ie. more than 2^32 "Save" operations), it can lead to the private keys being exposed.

See: #6027
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

* Changed per review feedback

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

---------

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

* Add content-length to http channel (#7537)

* add content-length to http channel

Signed-off-by: yaron2 <schneider.yaron@live.com>

* update tests

Signed-off-by: yaron2 <schneider.yaron@live.com>

---------

Signed-off-by: yaron2 <schneider.yaron@live.com>

* Updates components-contrib to 1.13.0-rc.4 (#7538)

* Updates components-contrib to 1.13.0-rc.3

Signed-off-by: joshvanl <me@joshvanl.dev>

* mod-tidy-all

Signed-off-by: joshvanl <me@joshvanl.dev>

---------

Signed-off-by: joshvanl <me@joshvanl.dev>
Co-authored-by: Yaron Schneider <schneider.yaron@live.com>

* Subscriptions: Fix panic when match rule is empty (#7539)

Prevents a panic resulting when a subscription route rule match
(`spec.routes.rules.match`) is empty. An empty match rule is valid and
is considered "default". Error occurs because of Go interface vs
implementation struct pointer nil check foot-gun.

Adds integration tests for both HTTP and gRPC subscribers.

Signed-off-by: joshvanl <me@joshvanl.dev>

* Hot Reloading: don't watch files if not enabled (#7521)

* Hot Reloading: don't watch files if not enabled

Update hot reloading so that we don't setup file watchers on the
component directory if hot reloading is not enabled.

Signed-off-by: joshvanl <me@joshvanl.dev>

* Remove `Close` paradigm from hot reloader

Signed-off-by: joshvanl <me@joshvanl.dev>

* Linting

Signed-off-by: joshvanl <me@joshvanl.dev>

---------

Signed-off-by: joshvanl <me@joshvanl.dev>
Co-authored-by: Yaron Schneider <schneider.yaron@live.com>

* Remove pubsub content-length test which has been removed from (#7550)

release-1.13

Signed-off-by: joshvanl <me@joshvanl.dev>

* Revert ApiLevel controlling vnodes back to context metadata (#7547)

* Revert ApiLevel controlling vnodes back to context metadata

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Lint

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Fixes after review

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Set back api level to 10, just for completeness purposes

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Don’t specify APILevelSpecify api level

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Apply suggestions from code review

Co-authored-by: Josh van Leeuwen <me@joshvanl.dev>
Signed-off-by: Elena Kolevska <elena-kolevska@users.noreply.github.com>

* Cleanup after review

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Small refactor

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Refactor multiple parameters into a request object for placement table dissemination

Signed-off-by: Elena Kolevska <elena@kolevska.com>

---------

Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena-kolevska@users.noreply.github.com>
Co-authored-by: Josh van Leeuwen <me@joshvanl.dev>
Co-authored-by: Artur Souza <asouza.pro@gmail.com>

* Actor Reminders: Default JSON serialization. (#7548)

* Actor Reminders: Default JSON serialization.

To support downgrades to 1.12 from 1.13, this PR changes the reminder
serialization storage format back to JSON by default. This means a 1.12
actor reminder client can read reminders written by 1.13 actors.

1.13 will continue to understand both JSON and protobuf. Protobuf
serialization can be enabled with the `ActorReminderStorageProtobuf`
feature gate. The actor "API Level" has been changed back to 10.

Adds test to ensure the default serialization is JSON.

Signed-off-by: joshvanl <me@joshvanl.dev>

* Remove ActorReminderStorageProtobuf feature gate in favour of using API
level

Signed-off-by: joshvanl <me@joshvanl.dev>

* Fix api level tests

Signed-off-by: joshvanl <me@joshvanl.dev>

---------

Signed-off-by: joshvanl <me@joshvanl.dev>
Co-authored-by: Yaron Schneider <schneider.yaron@live.com>

* Update contrib to 1.13.0-rc.6 (#7553)

* update contrib to 1.13.0-rc.5

Signed-off-by: yaron2 <schneider.yaron@live.com>

* update to rc.6

Signed-off-by: yaron2 <schneider.yaron@live.com>

---------

Signed-off-by: yaron2 <schneider.yaron@live.com>

* Updates components-contrib to 1.13.0-rc.7 (#7562)

Signed-off-by: joshvanl <me@joshvanl.dev>

* update contrib to 1.13.0-rc.8 (#7567)

* Add metadata in binding response even in case of error (#7572)

* Add binding metadata even in case of error.

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

* ADD IT.

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

* Fix lint.

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

---------

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

* [Release-1.13] Upgrade to contrib `v1.13.0-rc.10` (#7577)

* Upgrade to contrib 1.13.0-rc.9

Signed-off-by: Bernd Verst <github@bernd.dev>

* Use contrib 1.13.0-rc.10 for latest kafka sarama patch version

Signed-off-by: Bernd Verst <github@bernd.dev>

---------

Signed-off-by: Bernd Verst <github@bernd.dev>

* Fix for issue 7576 (#7581) (#7587)

Signed-off-by: Guido Spadotto <guido.spadotto@profesia.it>
Co-authored-by: Guido Spadotto <guido.spad8@gmail.com>
Co-authored-by: Guido Spadotto <guido.spadotto@profesia.it>

* Adds v1.13.0 release notes (#7586)

* Adds v1.13.0 release notes

Adds docs/release_notes/v1.13.0.md

Signed-off-by: joshvanl <me@joshvanl.dev>

* Update docs/release_notes/v1.13.0.md

Co-authored-by: Paul Yuknewicz <paulyuk@microsoft.com>
Signed-off-by: Josh van Leeuwen <me@joshvanl.dev>

* Move Actor Reminder Performance higher in notes

Signed-off-by: joshvanl <me@joshvanl.dev>

* Update v1.13.0.md

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

---------

Signed-off-by: joshvanl <me@joshvanl.dev>
Signed-off-by: Josh van Leeuwen <me@joshvanl.dev>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Co-authored-by: Paul Yuknewicz <paulyuk@microsoft.com>
Co-authored-by: Artur Souza <asouza.pro@gmail.com>

* chore: bump go to 1.21.8 and protobuf lib to 1.33.0 (#7591)

* ci: force go1.21.8 in workflows instead of go.mod

Signed-off-by: mikeee <hey@mike.ee>

* chore: bump google.golang.org/protobuf to 1.33.0

Signed-off-by: mikeee <hey@mike.ee>

* make modtidy-all

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

---------

Signed-off-by: mikeee <hey@mike.ee>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Co-authored-by: Artur Souza <asouza.pro@gmail.com>

---------

Signed-off-by: yaron2 <schneider.yaron@live.com>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: joshvanl <me@joshvanl.dev>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena-kolevska@users.noreply.github.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Signed-off-by: Bernd Verst <github@bernd.dev>
Signed-off-by: Guido Spadotto <guido.spadotto@profesia.it>
Signed-off-by: Josh van Leeuwen <me@joshvanl.dev>
Signed-off-by: mikeee <hey@mike.ee>
Co-authored-by: Yaron Schneider <schneider.yaron@live.com>
Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Co-authored-by: Elena Kolevska <elena-kolevska@users.noreply.github.com>
Co-authored-by: Artur Souza <asouza.pro@gmail.com>
Co-authored-by: Bernd Verst <github@bernd.dev>
Co-authored-by: Guido Spadotto <guido.spad8@gmail.com>
Co-authored-by: Guido Spadotto <guido.spadotto@profesia.it>
Co-authored-by: Paul Yuknewicz <paulyuk@microsoft.com>
Co-authored-by: Mike Nguyen <hey@mike.ee>
JoshVanL added a commit to JoshVanL/dapr that referenced this pull request Mar 6, 2024
* Actor Reminders: Default JSON serialization.

To support downgrades to 1.12 from 1.13, this PR changes the reminder
serialization storage format back to JSON by default. This means a 1.12
actor reminder client can read reminders written by 1.13 actors.

1.13 will continue to understand both JSON and protobuf. Protobuf
serialization can be enabled with the `ActorReminderStorageProtobuf`
feature gate. The actor "API Level" has been changed back to 10.

Adds test to ensure the default serialization is JSON.

Signed-off-by: joshvanl <me@joshvanl.dev>

* Remove ActorReminderStorageProtobuf feature gate in favour of using API
level

Signed-off-by: joshvanl <me@joshvanl.dev>

* Fix api level tests

Signed-off-by: joshvanl <me@joshvanl.dev>

---------

Signed-off-by: joshvanl <me@joshvanl.dev>
Co-authored-by: Yaron Schneider <schneider.yaron@live.com>
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.

None yet

4 participants