Skip to content

Clean-up main model: job dispatch interface and adapter #1036

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

Merged
merged 45 commits into from
Aug 11, 2025

Conversation

figueroa1395
Copy link
Contributor

@figueroa1395 figueroa1395 commented Jul 14, 2025

Relates to #1063

Part 2 of the split-up of MainModelImpl into the following design: create the job_dispatch_interface and job_dispatch_adapter
Untitled

Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
@figueroa1395 figueroa1395 self-assigned this Jul 14, 2025
@figueroa1395 figueroa1395 added the do-not-merge This should not be merged label Jul 14, 2025
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
@figueroa1395 figueroa1395 changed the title Cleanup main model: batch dispatch interface and adaptor Cleanup main model: job dispatch interface and adaptor Jul 22, 2025
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
…-adapter-prototipe

Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
@figueroa1395 figueroa1395 force-pushed the feature/cleanup-main-model-batch-dispatch branch from ce332ac to 065f5b8 Compare July 22, 2025 08:08
@mgovers mgovers force-pushed the feature/cleanup-main-model-batch-dispatch branch from 065f5b8 to 436b9e7 Compare July 22, 2025 08:22
Signed-off-by: Martijn Govers <[email protected]>
@figueroa1395 figueroa1395 changed the title Cleanup main model: job dispatch interface and adaptor Cleanup main model: job dispatch interface and adapter Jul 23, 2025
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
@figueroa1395 figueroa1395 marked this pull request as ready for review July 25, 2025 08:08
@Jerry-Jinfeng-Guo
Copy link
Contributor

Is there already test cases that demonstrate how easily this new adapter-interface scheme can be tested with mocks?

@figueroa1395
Copy link
Contributor Author

Is there already test cases that demonstrate how easily this new adapter-interface scheme can be tested with mocks?

Not yet. That would be part 3 of the saga. But in summary, we just need to test the job_dispatch calls (which should be easy to mock - just make a fake adapter that "implements" the interface and the result/update datasets can be "empty" structs or something simple), and then potentially evaluate if we need to test the job_dispatch_adapter or to what extend to do so, as it is mostly a collection of calls to main model (which should be unit tested); probably constructors and such might need testing and if we do some actual relevant logic as well.

Signed-off-by: Santiago Figueroa Manrique <[email protected]>
@petersalemink95
Copy link
Member

Just went through the PR - was quite hard to review. Not complaining - it is inherit to the nature of the PR -> shifting around code 😄

That said: I don't see anything weird in the structure, so if both @figueroa1395 and @mgovers are happy with the content it would be good to go for me as well

Signed-off-by: Nitish Bharambe <[email protected]>
Signed-off-by: Nitish Bharambe <[email protected]>
Copy link
Contributor

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo left a comment

Choose a reason for hiding this comment

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

I know that most of the requires come from existing code, but when would be a good moment to replace them with named concepts?

@figueroa1395
Copy link
Contributor Author

I know that most of the requires come from existing code, but when would be a good moment to replace them with named concepts?

It is good that you raised this question, because in a follow up PR (#1071) I decided to silence sonar about that, so maybe that isn't the right approach.

My thought process was that these requieres clauses are very specific to this interface, and we probably won't ever use them elsewhere, so I don't think it is worth making them a named concept. On the other hand, if it would add more clarity or you have a different take, we can do it. However, if we change them, I would do it at #1071, as the concepts in the interface are cleaned up a bit, so it's probably a good place to deal with that.

nitbharambe
nitbharambe previously approved these changes Aug 8, 2025
Copy link
Member

@nitbharambe nitbharambe left a comment

Choose a reason for hiding this comment

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

LGTM post resolving of all comments.

Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Copy link

@nitbharambe nitbharambe enabled auto-merge August 11, 2025 12:51
@nitbharambe nitbharambe added this pull request to the merge queue Aug 11, 2025
Merged via the queue into main with commit 82b4de3 Aug 11, 2025
31 checks passed
@nitbharambe nitbharambe deleted the virtual-adapter-prototipe branch August 11, 2025 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement on internal implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants