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

Prod Release 15/02/24 #561

Merged
merged 12 commits into from
Feb 15, 2024
Merged

Prod Release 15/02/24 #561

merged 12 commits into from
Feb 15, 2024

Conversation

morgsmccauley
Copy link
Collaborator

morgsmccauley and others added 11 commits February 7, 2024 11:21
The start block parameter (`start_block_height`) has two states,
signifying "Start from block" and "Start from latest" only. With the
removal of the continuous "real-time" process, which essentially runs
Indexers "From Interruption", the need for a third state arises,
representing "Interruption" arises. This PR expands the registry
contract/types to accommodate this change. This work was a good
opportunity to refactor the existing types, removing unnecessary fields
and tailoring it to the new architecture.

## Registry Types Refactoring
- `filter: IndexerRule` -> `rule: Rule`: `IndexerRule` contained a lot
of noise/data which was not needed. `id`, `name`, and
`indexer_rule_kind` have all been removed. The only useful part here is
`MatchingRule` which has been renamed to just `Rule`.
- `schema` is now non-optional: This field is always required so it
makes sense to convey that in the code. Having it as an `Option` created
lots of unnecessary checks throughout the system.
- `start_block_height` replaced by `start_block`: The latter being an
`enum` to represent "From Latest", "From Block", and "From Interruption"

## Public methods/API
To minimise the disruption of the existing contract consumers, the
public API remains unchanged, i.e. the core methods
(`list_indexer_functions`, `register_indexer_function`, etc.) use/return
the same types. As the underlying data will be migrated to the new
types, these methods infer the old types from the new ones. New methods
have been created to work with the new types (`register`, `list_all`,
etc.) allowing clients to move over when possible, as opposed to
creating a breaking change.
feat: Add Metrics for Memory Statistics
When an Executor is stopped, the metrics exposed from that worker still
persist. As metrics are aggregated across all workers, the metrics for a
given indexer would be aggregated across all previous workers, creating
incorrect metrics.
`xread` returns messages _after_ the provided ID, there's no need to
increment the ID ourselves. If we increment to an ID that actually
exists within the stream, the message will be skipped permanently. e.g.
we have messages `1-0` and `1-1`, on first read we get `1-0`, we
increment to `1-1`, then on the second read we get nothing.
…#553)

This PR adds support for the updated Registry types, including the new
`StartBlock` config option, across the Block Streamer/Coordinator.

With `StartBlock`, the logic within Coordinator is much more straight
forward - we no longer need to guess whether we should "continue" or
"start over", that is build in to the configuration options.

This only really affects the handling of Block Streams, the executor
flow remains the same: always restart when a new version is published.

To summarise the Block Stream synchronisation process:
- `StartBlock::Continue` - Resumes process, keeping the current data in
the Redis Stream, and start the Block Stream from `last_published_block`
- `StartBlock::Latest` - Starts a new process, clears the Redis Stream,
starts the Block Stream from the registry version, essentially being
latest
- `StartBlock::Height(u64)` - Starts a new process, clears the Redis
Stream, starts the Block Stream from the height configured

Additionally, Accounts/Indexers which have just been migrated, and also
streams which have been stopped but have unchanged versions (e.g. after
Block Streamer restart), will be treated the same as
`StartBlock::Continue`.
@morgsmccauley morgsmccauley requested a review from a team as a code owner February 14, 2024 21:57
A line to throw the error was mistakenly added during testing of things.
This should not be thrown since it prevents retries.
Copy link
Collaborator

@darunrs darunrs left a comment

Choose a reason for hiding this comment

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

I think it all looks good to me. Two comments on the changes before we send them out.

storage::get(redis_connection_manager, storage::DENYLIST_KEY).await?;
let raw_denylist: String = storage::get(redis_connection_manager, storage::DENYLIST_KEY)
.await
.unwrap_or("".to_owned());
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be returning "[]" in order to not cause problems in fact. I can either push a quick PR to merge that change or we can let it through, since it isn't a problem if there's a denylist anyway (You can add it in redis before merging).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

[] has already been added to Redis :)

#[derive(BorshDeserialize, BorshSerialize, Debug)]
pub struct Contract {
registry: IndexersByAccount,
pub struct OldContract {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you deployed the updates to prod's registry already?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup 👍🏼

@morgsmccauley morgsmccauley merged commit 895b761 into stable Feb 15, 2024
22 checks passed
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