feat(listener): use available cores to help other CUs #2163
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
Member
justprosh
commented
Mar 12, 2024
•
justprosh
commented
- improve logs in case of subscription failure
- use released ccp crates
- all units not involved in deals participate in CC calculation
folex
reviewed
Mar 12, 2024
| persisted_proof_id_dir: PathBuf, | ||
|
|
||
| unit_activated: Option<Subscription<Log>>, | ||
| unit_deactivated: Option<Subscription<Log>>, | ||
| unit_activated: Option<Subscription<JsonValue>>, |
There was a problem hiding this comment.
damn. Any way to add specific type there? Subscription to json doesn't sound very appealing :D
maybe you can .map it, and store mapped?
folex
reviewed
Mar 12, 2024
Comment on lines
679
to
+685
| let event = event.ok_or(eyre!( | ||
| "Failed to process CommitmentActivated event: got None" | ||
| ))?; | ||
| let cc_event = parse_log::<CommitmentActivatedData, CommitmentActivated>(event?)?; | ||
| ))??; | ||
| let log = serde_json::from_value::<Log>(event.clone()).map_err(|err| { | ||
| tracing::error!(target: "chain-listener", "Failed to parse CommitmentActivated event: {err}, data: {event}"); | ||
| err | ||
| })?; |
folex
reviewed
Mar 12, 2024
| } else { | ||
| // Will be activated on the `start_epoch` | ||
| self.pending_compute_units.insert(unit_event.info.into()); | ||
| } | ||
|
|
||
| self.refresh_commitment().await?; |
There was a problem hiding this comment.
let's add a comment on why you do it here
folex
reviewed
Mar 12, 2024
| @@ -786,11 +808,43 @@ impl ChainListener { | |||
| tracing::info!(target: "chain-listener", "Global nonce: {}", self.global_nonce); | |||
| tracing::info!(target: "chain-listener", "Difficulty: {}", self.difficulty); | |||
| if let Some(ref ccp_client) = self.ccp_client { | |||
| let cores = self.acquire_active_units()?; | |||
| let mut cores = self.acquire_active_units()?; | |||
There was a problem hiding this comment.
cores... what? free, busy, to be used?
folex
reviewed
Mar 12, 2024
Comment on lines
+828
to
+834
| for unit in self.active_compute_units.iter().cycle() { | ||
| if let Some(core) = available_cores.pop_first() { | ||
| cores.insert(core, *unit); | ||
| } else { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
what happens here? let's leave comments
folex
reviewed
Mar 12, 2024
Comment on lines
+846
to
+847
| self.global_nonce, | ||
| self.difficulty, |
folex
reviewed
Mar 12, 2024
| @@ -832,9 +881,11 @@ impl ChainListener { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| /// Should be called only if Commitment is Inactive, Failed, Removed or not exists | |||
There was a problem hiding this comment.
can you add debug_assert on that?
folex
reviewed
Mar 12, 2024
Comment on lines
1120
to
+1121
| } | ||
| fn get_available_cores(&self) -> eyre::Result<BTreeSet<PhysicalCoreId>> { |
There was a problem hiding this comment.
Suggested change
| } | |
| fn get_available_cores(&self) -> eyre::Result<BTreeSet<PhysicalCoreId>> { | |
| } | |
| fn get_available_cores(&self) -> eyre::Result<BTreeSet<PhysicalCoreId>> { |
folex
reviewed
Mar 12, 2024
| self.core_manager.acquire_worker_core(AcquireRequest::new(available_units, WorkType::CapacityCommitment)) | ||
| .map(|acquired| acquired.physical_core_ids) | ||
| .map_err(|err| { | ||
| tracing::error!(target: "chain-listener", "Failed to acquire cores for active units: {err}"); |
There was a problem hiding this comment.
are the units active, pending or available? I'm confused:
let available = self.pending
log("failed to acquire for active")
too many names, and I don't see how they can be composed
folex
reviewed
Mar 12, 2024
| @@ -1069,4 +1118,13 @@ impl ChainListener { | |||
| self.active_deals.remove(deal_id); | |||
| Ok(()) | |||
| } | |||
| fn get_available_cores(&self) -> eyre::Result<BTreeSet<PhysicalCoreId>> { | |||
There was a problem hiding this comment.
What does this function do?
Name suggests it's effectless, but body suggests it affects CoreManager's state.
|
Comments will be addressed in next PR |
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.