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

bgs: proposed changes from review #356

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

bnewbold
Copy link
Collaborator

Had a couple proposed changes from reviewing this code, was easiest to just edit locally and send a PR. feel free to keep or drop anything here.

other review notes:

Do we need to do the blob sync stuff at all in BGS? not sure that is in-scope. maybe it is disabled by not configuring when deployed, but could just rip the code out also.

As a high-level thing, many error cases result in err getting returned, but I believe we just log and move on. Maybe event processing should have multiple high-level results:

  • success: processed and emitted an event
  • skip: intentionally did not process event, and a downstream event was not emitted. eg, takedowns, some categories of invalid event
  • dirty: event should have been processed, but failed, in a way that leaves repo in partially-processed state. could flag the repo as dirty and needing cleanup. eg, identity resolution timeout
  • error: bad overall error, like disk full or database connection lost. instead of rapidly erroring on many sequential events, should probably just crash or halt

kind of an architecture philosophy thing and maybe you want to stick with the current system (pros/cons), but as we firm up all the possible validation error cases a framework like the above might be helpful.

Copy link
Collaborator

@ericvolp12 ericvolp12 left a comment

Choose a reason for hiding this comment

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

Seems legit to me

@ericvolp12
Copy link
Collaborator

Makes me wonder if we want to be forwarding event times produced by the PDS or if we should be recreating the timestamp with when we saw them...

@bnewbold bnewbold marked this pull request as draft December 20, 2023 12:08
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.

None yet

3 participants