Enhance Jira plugin: add workers configuration, optimize issue listing, and improve connection handling #168
Conversation
…g, and improve connection handling
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: Copilot <[email protected]>
…PI requests and update documentation accordingly
|
A few questions after reviewing the PR:
|
jira/table_jira_issue.go
Outdated
| return nil, nil | ||
| } | ||
|
|
||
| var starts []int |
There was a problem hiding this comment.
why is this needed? can't we just use these loop params on line 288?
There was a problem hiding this comment.
Should the parallel page retrieval functionality be integrated into the plugin SDK instead of being handled on a per table basis?
Possibly - but I would tend to wait until we establish a need/use case across multiple tables/plugins before making an API change
| "epic": getFieldKey(ctx, d, names, "Epic Link"), | ||
| "sprint": getFieldKey(ctx, d, names, "Sprint"), | ||
| } | ||
| d.StreamListItem(ctx, IssueInfo{issue, keys}) |
There was a problem hiding this comment.
The SDK is not expecting concurrent calls to this function, so locking is recommended
There was a problem hiding this comment.
ON the whole I think this PR seems OK however as mentioned, StreamListItem was not intended to be called in parallel so there is potential for race conditions within the state management code.
SImplest way to fix would probably be to put a mutex lock around the call to StreamListItem
|
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 30 days. |
github-actions
bot
added
the
stale
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the Jira plugin by adding configurable worker concurrency, optimizing issue listing with dynamic field selection, and improving connection handling with better variable naming and error management.
- Adds configurable workers parameter to enable parallel API requests for better performance
- Optimizes issue listing by implementing dynamic field selection and parallel processing with goroutines
- Improves code quality with better variable naming (snake_case to camelCase) and cleaner connection handling
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| jira/utils.go | Adds worker management functions, dynamic field selection, and improves variable naming in connection handling |
| jira/table_jira_issue.go | Implements parallel processing for issue listing with worker pools and optimizes API requests |
| jira/table_jira_issue_worklog.go | Fixes variable reference bug in worklog streaming |
| jira/connection_config.go | Adds workers configuration field to connection config |
| docs/index.md | Documents the new workers configuration parameter |
| config/jira.spc | Updates sample configuration with workers parameter |
|
|
||
| d.ConnectionManager.Cache.Set(cacheKey, maxWorkers) | ||
|
|
||
| plugin.Logger(ctx).Warn("getMaxWorkers", "found_workers", maxWorkers) |
There was a problem hiding this comment.
Using Warn level logging for normal configuration information is inappropriate. This should use Debug or Info level instead, as finding workers configuration is not a warning condition.
| plugin.Logger(ctx).Warn("getMaxWorkers", "found_workers", maxWorkers) | |
| plugin.Logger(ctx).Info("getMaxWorkers", "found_workers", maxWorkers) |
Copilot uses AI. Check for mistakes.
| @@ -244,75 +245,92 @@ func tableIssue(_ context.Context) *plugin.Table { | |||
|
|
|||
| //// LIST FUNCTION | |||
|
|
|||
| // The listIssues function first makes an initial request to determine the total number of issues without expanding any fields for efficiency. Subsequent paginated requests use the 'names' expansion to retrieve batches of issues with field names resolved. | |||
There was a problem hiding this comment.
The comment describes using 'names' expansion in subsequent requests, but the code actually uses 'names' expansion for all paginated requests, not just subsequent ones. The comment should be updated to accurately reflect the implementation.
| // The listIssues function first makes an initial request to determine the total number of issues without expanding any fields for efficiency. Subsequent paginated requests use the 'names' expansion to retrieve batches of issues with field names resolved. | |
| // The listIssues function first makes an initial request to determine the total number of issues without expanding any fields for efficiency. All subsequent paginated requests use the 'names' expansion to retrieve batches of issues with field names resolved. |
Copilot uses AI. Check for mistakes.
| } | ||
| mu.Lock() | ||
| d.StreamListItem(ctx, IssueInfo{issue, keys}) | ||
| mu.Unlock() |
There was a problem hiding this comment.
The mutex is held while calling d.StreamListItem() which could be a blocking operation. This reduces the benefits of parallel processing. Consider using a channel to collect results and stream them from the main goroutine instead.
Copilot uses AI. Check for mistakes.
| d.StreamListItem(ctx, IssueInfo{issue, keys}) | ||
| mu.Unlock() | ||
|
|
||
| if d.RowsRemaining(ctx) == 0 { |
There was a problem hiding this comment.
Checking d.RowsRemaining() inside the goroutine while holding a mutex may not accurately reflect the global state across all workers. This could lead to race conditions where multiple workers continue processing after the limit is reached.
Copilot uses AI. Check for mistakes.
github-actions
bot
removed
the
stale
Example query results
Results