refactor: bidder to simplfiy exposing errors #3680
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThis update streamlines the bidding and execution logic for compute resources, enhancing efficiency and clarity. It restructures the Changes
Related issues
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
frrist
force-pushed
the
frrist/job-history-comments
branch
from
85013cf
to
1596ae7
Compare
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (11)
- pkg/bidstrategy/resource/capacity_available_strategy.go (3 hunks)
- pkg/bidstrategy/resource/capacity_max_strategy.go (2 hunks)
- pkg/bidstrategy/semantic/networking.go (1 hunks)
- pkg/compute/bidder.go (3 hunks)
- pkg/compute/bidder_test.go (6 hunks)
- pkg/compute/endpoint.go (2 hunks)
- pkg/compute/types.go (2 hunks)
- pkg/node/compute.go (3 hunks)
- pkg/requester/endpoint.go (2 hunks)
- pkg/test/compute/async_bid_test.go (3 hunks)
- pkg/test/compute/setup_test.go (2 hunks)
Additional comments: 21
pkg/bidstrategy/semantic/networking.go (1)
- 20-20: The TODO comment regarding the invalid link in the
docsLinkconstant is clear and actionable. It's good practice to track such TODOs in a project management tool or issue tracker to ensure they are addressed in a timely manner.pkg/bidstrategy/resource/capacity_max_strategy.go (2)
- 5-5: The addition of the
fmtimport is appropriate for the functionality introduced in theShouldBidBasedOnUsagemethod.- 27-38: The refactored logic in the
ShouldBidBasedOnUsagemethod, including the conditional check on resource availability and the construction ofBidStrategyResponse, improves the clarity and functionality of the method.pkg/bidstrategy/resource/capacity_available_strategy.go (3)
- 5-5: The addition of the
fmtimport is appropriate for the functionality introduced in theShouldBidBasedOnUsagemethod.- 22-22: The removal of the
ctxparameter fromNewAvailableCapacityStrategysimplifies the function signature, making it more straightforward to use.- 35-46: The updated logic in the
ShouldBidBasedOnUsagemethod, including the conditional check on available capacity and the construction ofBidStrategyResponse, improves the clarity and functionality of the method.pkg/test/compute/async_bid_test.go (1)
- 75-106: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [81-125]
The introduction and usage of
sync.WaitGroupinstances (doneWgandbidSentWg) in the asynchronous bid tests improve synchronization and coordination, ensuring proper sequencing and handling of bid responses. This is a good practice for managing the flow of asynchronous operations in tests.pkg/test/compute/setup_test.go (1)
- 71-71: Initializing the
bidChannelwith a buffer size of 1 is a good practice for preventing potential deadlocks in tests involving asynchronous operations. This allows for non-blocking sends to the channel, improving the reliability of the test setup.pkg/compute/types.go (1)
- 146-146: The addition of the
Waitfield to theBidResultstruct enhances the flexibility and control in the bidding process, allowing for more nuanced decision-making by the compute node.pkg/compute/endpoint.go (3)
- 7-9: Reordering import statements improves readability by grouping similar imports together.
- 60-68: Adding error handling for parsing job resource configuration in the
AskForBidfunction is a necessary improvement for robustness, ensuring that errors are properly logged and communicated.- 70-76: The restructuring of the
AskForBidfunction to include parsing of job resource config and passing it toRunBiddingenhances the functionality and clarity of the bidding process.pkg/compute/bidder_test.go (2)
- 58-63: Changing
SemanticStrategyandResourceStrategyto slices and updating theUsageCalculatorinitialization improves flexibility, extensibility, and cohesion in the bidder tests setup.- 157-162: Adjusting the
RunBiddingmethod calls to pass aBidderRequeststruct simplifies the method signature and improves code readability in the bidder tests.pkg/node/compute.go (2)
- 146-158: Refactoring the bidder creation into a separate
NewBidderfunction simplifies theNewComputeNodefunction and improves code modularity. Ensure that all necessary parameters are correctly passed toNewBidder.- 253-328: The implementation of
NewBiddercorrectly encapsulates the bidder creation logic, enhancing code modularity and readability. Ensure that it integrates well with the rest of the system and that all parameters are utilized appropriately.pkg/requester/endpoint.go (2)
- 248-248: Adding the
Commentfield to capture the reason for bid completion enhances clarity and traceability. Ensure thatresponse.Reasonprovides meaningful information and integrates well with existing mechanisms.- 343-343: Adding the
Commentfield to capture the reason for compute failures enhances clarity and traceability. Ensure thatresult.Errprovides meaningful information and integrates well with existing mechanisms.pkg/compute/bidder.go (3)
- 22-40: The restructuring of the
Bidderstruct to includeSemanticStrategy,ResourceStrategy, andUsageCalculatorsupports a more modular and simplified bidding process. Ensure that these new fields are utilized appropriately and integrate well with the rest of the system.- 252-300: The implementation of
runSemanticBiddingcorrectly encapsulates semantic bidding logic, enhancing code modularity. Ensure that it properly evaluates semantic strategies and integrates well with error handling and logging mechanisms.- 302-360: The implementation of
runResourceBiddingcorrectly encapsulates resource bidding logic, enhancing code modularity. Ensure that it properly calculates resource usage, evaluates resource strategies, and integrates well with error handling and logging mechanisms.
rossjones
reviewed
Mar 22, 2024
rossjones
reviewed
Mar 28, 2024
Comment on lines
+254
to
+261
| config ComputeConfig, | ||
| publishers publisher.PublisherProvider, | ||
| storages storage.StorageProvider, | ||
| executors executor.ExecutorProvider, | ||
| runningCapacityTracker capacity.Tracker, | ||
| enqueuedCapacityTracker capacity.Tracker, | ||
| nodeID string, | ||
| executionStore store.ExecutionStore, | ||
| computeCallback compute.Callback, | ||
| bufferRunner *compute.ExecutorBuffer, | ||
| apiServer *publicapi.Server, | ||
| calculator capacity.UsageCalculator, |
There was a problem hiding this comment.
When should we use Params and when should we just inline all the parameters.
There was a problem hiding this comment.
Yeah, this brings up a good point. We should probably give our usage of the *Params pattern more thought. It makes it easy to forget to pass values to a constructor when things change unless we inline them as you've said e.g
// can panic when a new field is added to params but not populated in the constructor
svr := NewServer(ServerParams{
Host: "0.0.0.0",
Port: 80,
Config: []byte("whatever"),
})
// will fail to compile if a new field is added to params
svr := NewServer(ServerParams{
"0.0.0.0",
80,
[]byte("whatever"),
})I believe it is considered a best practice to provide field names when initializing a structure, but the catch is we lose compile time guarantees when they change. The NewBidder method here accepts all arguments required to construct bidding and helps reduce the cognitive load of the NewComputeNode constructor, okay if this is left as is?
I think the initial intent of the *Params pattern was to make it easier to adopt a dependency injection framework (do I remember this correctly @wdbaruni?). For example, if we use uber.fx *Params are easy to migrate to: https://uber-go.github.io/fx/parameter-objects.html#using-parameter-objects
There was a problem hiding this comment.
I do think this is okay for now, really just highlighting that it might be a good candidate for a future PR
frrist
force-pushed
the
frrist/job-history-comments
branch
from
5a398ad
to
0e519e8
Compare
added 8 commits
April 2, 2024 08:53
In a commit to follow it will be simpler to surface bidding failures to the compute node.
This commit simplifies the bidder's implementation with the following key changes: - We've removed the use of network transport types for internal communications, which previously added unnecessary complexity. - It's important to note that the responsibilities of the bidder significantly exceed its basic function of deciding whether to bid on jobs. The current scope includes a range of tasks such as updating execution states, initiating execution processes, making callbacks to the requester node, calculating job resource requirements, and managing job approval processes. The utility and continuation of some features, like job approval, are still under consideration due to lack of documentation. For illustration, refer to the setup in `bidder_test.go`. - Recognizing the overstretched functionality of the bidder, this refactor does not aim to comprehensively overhaul it but makes a significant stride towards future refactoring. By centralizing the aforementioned complexities into a single method, `handleBidResult`, we lay the groundwork for shaving the yak latter. This change does not shave the full yak, but has removed some hair.
- the executor will already handle this internally.
- semantic should be first because: https://github.com/bacalhau-project/bacalhau/pull/3680/files#r1543761175
frrist
force-pushed
the
frrist/job-history-comments
branch
from
0e519e8
to
44384e4
Compare
aronchick
pushed a commit
that referenced
this pull request
Apr 27, 2024
Refactors bidder to better expose errors closes https://github.com/bacalhau-project/expanso-planning/issues/389 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced bidding strategies to better handle available capacity and resource availability. - Introduced a new field to improve compute node bidding logic. - **Refactor** - Streamlined bidding process by restructuring the `Bidder` struct and related methods. - Simplified compute node setup with new bidder initialization function. - Adjusted test setups and synchronization for improved testing reliability. - **Bug Fixes** - Added error handling for job resource configuration parsing. - Updated endpoint methods to properly handle bid completion and compute failures. - **Chores** - Cleaned up import statements and added a TODO for documentation review. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: frrist <forrest@expanso.io>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactors bidder to better expose errors
closes #3868
Summary by CodeRabbit
New Features
Refactor
Bidderstruct and related methods.Bug Fixes
Chores