Skip to content
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

Fuse the calls to searchForIssuesUsingJql when possible + memoization + cleanups #1456

Merged
merged 12 commits into from
May 14, 2024

Conversation

ypc-faros
Copy link
Contributor

@ypc-faros ypc-faros commented May 13, 2024

Splits the former getIssues into 3 functions:

  • getIssuesKeys: only fetches issue key(s) => used only by the faros_board_issues stream
  • getIssues: fetches all the fields needed for the faros_issues stream (if requested) and all the fields needed for the faros_issue_pull_requests (if requested). This function also stores a projection of the issue (only the fields needed for faros_issue_pull_requests) in memory (seenIssues). I decided to use my own data structure vs @Memoized so that the memory usage was minimized.
  • getIssuesCompact: fetches the fields needed for faros_issue_pull_requests (from memory or from the api)

Aside from the above, a couple of minor changes:

  • Made sure that the jql(s) for getIssues and getIssuesCompact match, by storing start_date and end_date in the "global" config.
  • Major cleanup in how we calculate which fields should be fetched, depending on the streams requested.
  • Bug fix in the toposort (the reverse was not needed)

@ypc-faros ypc-faros changed the title Ypc/jira memo Fuse the calls to searchForIssuesUsingJql when possible + memoization + cleanups May 13, 2024
@ypc-faros ypc-faros marked this pull request as ready for review May 13, 2024 17:26
@@ -52,6 +54,9 @@ export interface JiraConfig extends AirbyteConfig {
readonly api_url?: string;
readonly api_key?: string;
readonly graph?: string;
readonly requestedStreams?: Set<string>;
Copy link
Contributor

Choose a reason for hiding this comment

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

feels like a leaky abstraction

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah maybe I over optimized there...

Another way we can do this is use config.run_mode for this purpose

If RunMode.WebhookSupplement, faros_issues is NOT requested (since it's not offered)
Otherwise (Full), assume both faros_issues and faros_issue_pull_requests are requested

99/100 times syncs will run with all supported streams, so this optimization is valuable in a small number of cases (the user would have to purposely disable one of these streams)

Copy link
Contributor

Choose a reason for hiding this comment

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

I like more the option of using the run_mode. We also avoid adding yet another config

return issueTransformer.toIssue(item);
},
'issues'
);
}

private memoizeIssue(item: any, jql: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a way we can generalize this? We kinda have the same need for Sprint and Sprint Reports so wondering if we would also need to do the same for that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe..., let's chat offline

Copy link

sonarcloud bot commented May 14, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@ypc-faros ypc-faros merged commit e2664af into main May 14, 2024
7 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators May 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants