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

[Fixes 4035] Add the hook.Code to the ModuleInvocationContext. #4036

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

scr-oath
Copy link
Contributor

@scr-oath scr-oath commented Nov 4, 2024

[Fixes #4035] Add the hook.Code to the ModuleInvocationContext.

@bretg
Copy link
Contributor

bretg commented Nov 15, 2024

@scr-oath - we discussed this in committee - can you please provide an example of how this addresses the issue? How is this helpful to your usecase?

@scr-oath
Copy link
Contributor Author

scr-oath commented Dec 13, 2024

Well, there was some example in the issue this references, but imagine having a stage that has three groups…

  1. module 1 one fires off async things
  2. other modules do their work unrelated to and not blocking 1 and make mutations with their findings
  3. module 1 joins the async + the mutations in 2 and returns further mutations.

Because the module 1 needs to have information about the in flight request passed to "itself" in group 3, and needs to know that it's the 2nd invocation, passing the hookImplCode to the module invocation itself can be helpful to the module knowing which group it's in. While one might be able to pass information to subsequent stages in the returned ModuleContext, they would need to know that they could NOT use that technique in the BidderRequst and RawBidderResponse stages without getting panics due to the lack of locks being held on reading map entries while they're being updated by other bidder responses.

Tell me, what was the intended purpose of the hookImplCode if not passed to the hook for consideration (where is it used other than logging)? And what is the fear of adding to ModuleInvocationContext ?

@bretg
Copy link
Contributor

bretg commented Dec 13, 2024

In my mind it was not intended for a module to be called twice in the same stage. Rather, the intention was that Module 1 could fire off it's async thing early in the workflow (e.g. raw auction request) and then utilize the results later (e.g. processed auction request)

If the module needs the results within the same stage, then it should just wait for the event, and not necessarily be split into two groups within the stage.

That said, the details of the feature was designed with Java VertX in mind by people who aren't here anymore. Perhaps there's something about Go that makes the architecture more awkward? From the issue:

if there is any I/O that can be started in one group and joined/Wait'ed in the 2nd - then the I/O time can overlap/be parallel with the CPU time of the first group.

The implication here is that Go (or Java) won't allow a module function to block on an I/O event without spinning CPU. If that's true, I've no objection to expanding the model.

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.

hook_impl_code should be available to a module in the ModuleInvocationContext
2 participants