Skip to content

Conversation

cyrossignol
Copy link

This avoids allocation of a superblock object from scraper stats objects when validating another superblock.

This avoids allocation of a superblock object from scraper stats objects
when validating another superblock.
Copy link
Owner

@jamescowens jamescowens left a comment

Choose a reason for hiding this comment

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

This looks really good. I guess the only disadvantage to this is the hashing "logic" is in two different places... but I don't really foresee this changing once we get it set...

@cyrossignol
Copy link
Author

cyrossignol commented Oct 13, 2019

Yeah, not a fan of the duplication either... I figured this would be worth it with the potential size of superblocks. I tried to limit duplication as much as possible to a well-defined interface and the unit test should catch cases where we forget to update one side.

Worst case we can just remove the custom hashing of ScraperStats if we ever get to the point where that hashing is incompatible or infeasible.

@jamescowens
Copy link
Owner

I think it is worth it. I am approving and merging.

@jamescowens jamescowens merged commit f477e20 into jamescowens:integrated_scraper_2 Oct 13, 2019
@cyrossignol cyrossignol deleted the integrated_scraper_2 branch February 25, 2020 05:51
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