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

Forward validations and manifests in reporting mode #3905

Closed
wants to merge 3 commits into from

Conversation

natenichols
Copy link
Contributor

High Level Overview of Change

Forwards validations and manifest subscription streams in reporting mode.

Context of Change

In order for reporting mode to be deployed to s1 and s2, it must support subscriptions to the validations and manifests streams.

This PR adds those streams in the same manner as transactions_proposed. validationReceived and manifestReceived messages are forwarded from the ETL Source to subscribers.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

Copy link
Contributor

@cjcobb23 cjcobb23 left a comment

Choose a reason for hiding this comment

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

Looks good, did you test this at all?

@natenichols
Copy link
Contributor Author

Yea, set it up locally and tried out the validations stream. Still need to test out the manifests

@cjcobb23
Copy link
Contributor

Can we hook it up to the validations-mainnet bot and make sure everything works correctly?

@natenichols
Copy link
Contributor Author

Sure, do you have an instance running anywhere? Or should I just set that up locally?

@cjcobb23
Copy link
Contributor

I don't have an instance; is it a pain to setup locally?

@natenichols
Copy link
Contributor Author

natenichols commented Aug 19, 2021

nah I just might not get to it til next week

Copy link
Contributor

@cjcobb23 cjcobb23 left a comment

Choose a reason for hiding this comment

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

I tested this. Everything looks good.

@@ -374,7 +374,7 @@ class ETLLoadBalancer
/// @param in ETLSource in question
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update the documentation for this function which currently only describes the transaction stream.

@@ -354,12 +358,30 @@ ETLSource::handleMessage()
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do the shouldPropagateStream() check once instead of in 3 places. Then, if the check passes, check for each of the stream types and proceed accordingly.

This removes duplicated code and I think makes the intention more clear.

Copy link
Collaborator

@mtrippled mtrippled left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@mtrippled mtrippled added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Sep 9, 2021
@nbougalis nbougalis mentioned this pull request Sep 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants