-
Notifications
You must be signed in to change notification settings - Fork 544
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
Submit snark work using graphql #16366
Conversation
!ci-build-me |
@@ -901,6 +901,13 @@ let add_work t (work : Snark_worker_lib.Work.Result.t) = | |||
(Network_pool.Snark_pool.Resource_pool.Diff.of_result work, cb) | |||
|> Deferred.don't_wait_for | |||
|
|||
let add_work_graphql t diff = | |||
let results_ivar = Ivar.create () in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is an ivar necessary here? Can we not just bind the deferred value and return?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure a an Ivar push makes sense as a validation call back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the purpose of this callback was to check the validation status and handle it appropriately for a given message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is an ivar necessary here? Can we not just bind the deferred value and return?
Yes, if you want to return the status of the operation- whether the completed work was broadcasted or not.
I thought the purpose of this callback was to check the validation status and handle it appropriately for a given message.
Not sure what the discrepancy is here. The validation status is returned to the grapqhl request and as for the message, it would either be added to the pool or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted to clarify one point about the validation callback before we merged this. I think the testing methodology is good.
!ci-build-me |
Explain your changes:
Explain how you tested your changes:
Part of solution described in #16214 (comment)
Checklist: