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

[KS-79] Remote trigger setup #12845

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

bolekk
Copy link
Contributor

@bolekk bolekk commented Apr 16, 2024

  1. Fully set up a (hardcoded) capability DON in Syncer and move fake Mercury producer over there.
  2. Refactor remote Aggregator interface to reduce marshaling steps.
  3. Avoid redundant calls to RegisterTrigger.
  4. Minor change to Prometheus config in Peer Wrapper.

Copy link
Contributor

I see you updated files related to core. Please run pnpm changeset in the root directory to add a changeset as well as in the text include at least one of the following tags:

#nops : For any feature that is NOP facing and needs to be in the official Release Notes for the release.
#added : For any new functionality added.
#changed : For any change to the existing functionality. 
#removed : For any functionality/config that is removed.
#updated : For any functionality that is updated.
#deprecation_notice : For any upcoming deprecation functionality.
#breaking_change : For any functionality that requires manual action for the node to boot.
#db_update : For any feature that introduces updates to database schema.
#wip : For any change that is not ready yet and external communication about it should be held off till it is feature complete.
#bugfix - For bug fixes.
#internal - For changesets that need to be excluded from the final changelog.

@bolekk bolekk force-pushed the feature/KS-79-remote-mercury-trigger branch from ee1804b to 5da4534 Compare April 16, 2024 17:57
@bolekk bolekk force-pushed the feature/KS-79-remote-mercury-trigger branch from 5da4534 to a30bb51 Compare April 16, 2024 18:25
@bolekk bolekk marked this pull request as ready for review April 16, 2024 18:26
@bolekk bolekk requested review from a team as code owners April 16, 2024 18:26
@bolekk bolekk force-pushed the feature/KS-79-remote-mercury-trigger branch from a30bb51 to 441bfc9 Compare April 16, 2024 18:41

agg := remote.NewDefaultModeAggregator(2)
_, err = agg.Aggregate("", [][]byte{marshaled1})
require.Error(t, err, "failed to aggregate responses, err: not enough identical responses found")
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer to make the error a type or var and check with assert/require ErrorIs rather than string matching.

this approach will also play nicely with wrapping errors via %w

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm I'm not sure how to do that and still support wrapped inner errors - do you have an example somewhere?
I also realized that the expected string matching here doesn't actually work that way so I'm removing it.

core/capabilities/syncer.go Outdated Show resolved Hide resolved
core/capabilities/syncer.go Show resolved Hide resolved
1. Fully set up a (hardcoded) capability DON in Syncer and move fake Mercury producer over there.
2. Refactor remote Aggregator interface to reduce marshaling steps.
3. Avoid redundant calls to RegisterTrigger.
4. Minor change to Prometheus config in Peer Wrapper.
@bolekk bolekk enabled auto-merge April 19, 2024 03:24
@bolekk bolekk added this pull request to the merge queue Apr 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 19, 2024
@cedric-cordenier cedric-cordenier added this pull request to the merge queue Apr 19, 2024
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.

4 participants