Skip to content

Implement concurrent targets in config command#328

Merged
harp-intel merged 6 commits into
mainfrom
threadedconfig
May 5, 2025
Merged

Implement concurrent targets in config command#328
harp-intel merged 6 commits into
mainfrom
threadedconfig

Conversation

@harp-intel
Copy link
Copy Markdown
Contributor

No description provided.

Signed-off-by: Harper, Jason M <jason.m.harper@intel.com>
@harp-intel harp-intel requested a review from Copilot May 4, 2025 00:57
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements concurrent processing of targets in the config command and updates related data structures. Key changes include:

  • Refactoring TargetScriptOutputs in common.go to use exported field names.
  • Converting synchronous configuration changes into concurrent operations with improved spinner usage and error reporting.
  • Enhancing the printConfig logic to process and display table outputs concurrently.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
internal/common/common.go Updated struct field names for TargetScriptOutputs for concurrency support.
cmd/config/config.go Refactored the config command to run target operations concurrently with improved status updates and error handling.
Comments suppressed due to low confidence (1)

internal/common/common.go:46

  • [nitpick] The renaming of the struct fields to exported names is clear; ensure that all consuming code and documentation are updated to reflect these changes.
type TargetScriptOutputs struct {
	TargetName    string
	ScriptOutputs map[string]script.ScriptOutput
	TableNames    []string
}

Comment thread cmd/config/config.go
harp-intel added 4 commits May 3, 2025 18:23
Signed-off-by: Harper, Jason M <jason.m.harper@intel.com>
Signed-off-by: Harper, Jason M <jason.m.harper@intel.com>
Signed-off-by: Harper, Jason M <jason.m.harper@intel.com>
Signed-off-by: Harper, Jason M <jason.m.harper@intel.com>
@harp-intel harp-intel requested a review from Copilot May 5, 2025 04:10
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements concurrent updates to the configuration command by refactoring flag functions and target operations to use asynchronous channel‐based error handling. Key changes include:

  • Renaming struct fields and updating accessors in internal/common/common.go to export necessary values.
  • Changing flag definitions and their corresponding set functions (in cmd/config/flag.go and flag_groups.go) to accept channel and index parameters to support concurrent execution.
  • Refactoring the configuration update and reporting logic in cmd/config/config.go to run updates concurrently using goroutines and channels.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
internal/common/common.go Updates exported field names and accessors for consistency.
cmd/config/flag_groups.go Adds an "Other Options" group and updates flag callback signatures.
cmd/config/flag.go Modifies flag functions to include concurrency parameters.
cmd/config/config.go Refactors config update logic for concurrent target processing.
Comments suppressed due to low confidence (2)

cmd/config/config.go:176

  • Consider assigning len(successMessages)-1 to a local variable before launching the goroutine to ensure the intended index is reliably captured.
go flag.intSetFunc(value, myTarget, localTempDir, channelSetComplete, len(successMessages)-1)

cmd/config/config.go:240

  • The error received from channelError is logged but not propagated back to the caller, which could lead to silent failures. Consider returning or otherwise handling the error to ensure proper error reporting.
select { case scriptOutputs := <-channelTargetScriptOutputs: ... case err := <-channelError: ... }

Signed-off-by: Harper, Jason M <jason.m.harper@intel.com>
@harp-intel harp-intel merged commit 16b4c13 into main May 5, 2025
4 checks passed
@harp-intel harp-intel deleted the threadedconfig branch May 5, 2025 16:22
echiugoog pushed a commit to echiugoog/PerfSpect that referenced this pull request May 21, 2025
* Implement concurrent targets in config command

Signed-off-by: Harper, Jason M <jason.m.harper@intel.com>

* Add detailed status messages for configuration changes on target

Signed-off-by: Harper, Jason M <jason.m.harper@intel.com>

* no-summary option

Signed-off-by: Harper, Jason M <jason.m.harper@intel.com>

* const str

Signed-off-by: Harper, Jason M <jason.m.harper@intel.com>

* parallelize the set functions

Signed-off-by: Harper, Jason M <jason.m.harper@intel.com>

* Refactor setOutput struct definition for clarity and consistency

Signed-off-by: Harper, Jason M <jason.m.harper@intel.com>

---------

Signed-off-by: Harper, Jason M <jason.m.harper@intel.com>
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.

2 participants