feat: ModuleContext periodically refreshed on the controller #1865
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jonathanj-square
requested review from
a team and
matt2e
and removed request for
a team
Open
jonathanj-square
force-pushed
the
jonathanj/controller-dynamic-module-context
branch
from
f48ae05 to
331d44f
Compare
alecthomas
approved these changes
Jun 25, 2024
ensure controller is live and db container is provisioned before executing startup tasks
`.Abs()` will return a clone with all paths made absolute. This avoids a bunch of manual and error prone joining.
- Looking into why we are getting ASM sync errors on staging and it is hard to tell whether the error is happening in the leader or the follower. These error changes should help this. - `secretsCache` should not have any mention of ASM as it is meant to be generic
By design, `RetryStreamingServerStream()` reissues the underlying RPC whenever the stream terminates, and `GetModuleContext()` was exiting immediately after sending its first response, resulting in a busy loop.
This required some changes to how the build engine works, in that build and deploy can now be run separately. Building the box: ``` 🐚 ~/dev/ftl $ ftl box echo examples/go info:time: Building module info:echo: Building module info: Building image echo ``` Running locally: ``` 🐚 ~/dev/ftl $ ftl box-run --dsn="postgres://postgres:secret@localhost:15432/ftl?sslmode=disable" examples/go/ info: Web console available at: http://0.0.0.0:8892 info: HTTP ingress server listening on: http://0.0.0.0:8891 info:time: Deploying module info: Reusing deployment: dpl-time-xpun6vddce07f5s info: Deployed dpl-echo-1bd1tvnmig06767u info: Deployed dpl-time-xpun6vddce07f5s info:echo: Deploying module info: Reusing deployment: dpl-echo-1bd1tvnmig06767u info: All modules deployed ``` Running the resulting image works but we need to set up Docker compose with PG: ``` 🐚 ~/dev/ftl $ docker run --platform linux/amd64 echo ftl: error: failed to create database: database not ready after 10 tries: failed to connect to `user=postgres database=`: 127.0.0.1:5432 (localhost): dial error: dial tcp 127.0.0.1:5432: connect: connection refused [::1]:5432 (localhost): dial error: dial tcp [::1]:5432: connect: connection refused ```
…being returned by the controller
wesbillman
reviewed
Jun 26, 2024
Comment on lines
+699
to
+700
| checksum = (checksum * 115163) + configurationMapChecksum(secrets) | ||
| checksum = (checksum * 454213) + configurationDatabaseChecksum(databases) |
There was a problem hiding this comment.
@jonathanj-square what are these 115163 and 454213 numbers?
There was a problem hiding this comment.
Agreed: just use sha256 for everything, there's no need to truncate it and switch to a manual 64-bit checksum. Pass around the hash.Hash from sha256.New() to generate the checksum, then just compare checksum.Sum() results with bytes.Compare()
alecthomas
reviewed
Jun 26, 2024
| // This operation is used to detect configuration change. | ||
| func configurationMapChecksum(m map[string][]byte) int64 { | ||
| sum := int64(0) | ||
| for k, v := range m { |
There was a problem hiding this comment.
Ranging over a map is not ordered, you'll need to use maps.Keys(m) and sort.Slice().
There was a problem hiding this comment.
Will make these adjustments in the follow-up PR. I went with a sum of hashes to avoid having to worry about ordering; after adopting the suggestions the randomized ordering will matter
There was a problem hiding this comment.
Ah yeah I realised after that you were adding the values to avoid the sort, but I think just using sha256 directly will be clearer and simpler.
| // This operation is used to detect configuration change. | ||
| func configurationDatabaseChecksum(m map[string]modulecontext.Database) int64 { | ||
| sum := int64(0) | ||
| for k, v := range m { |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #1699
GetModuleContextwill stream updatedModuleContextto the runner.