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

Deployed feed version summaries #516

Merged
merged 8 commits into from
Mar 16, 2023
Merged

Deployed feed version summaries #516

merged 8 commits into from
Mar 16, 2023

Conversation

br648
Copy link
Contributor

@br648 br648 commented Feb 28, 2023

Checklist

  • Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
  • Any modified or new methods or classes have helpful JavaDoc and code is thoroughly commented
  • The description lists all applicable issues this PR seeks to resolve
  • The description lists any configuration setting(s) that differ from the default settings
  • All tests and CI builds passing

Description

A new end point has been added to the feed source controller to return the deployed feed version for a given feed source. If the project has a pinned deployment and this deployment has a feed version this is used. If not, the latest feed version for a feed source is used.

@demory demory requested a review from philip-cline March 2, 2023 15:20
@philip-cline philip-cline assigned br648 and unassigned philip-cline Mar 7, 2023
@br648 br648 assigned philip-cline and unassigned br648 Mar 9, 2023
@br648
Copy link
Contributor Author

br648 commented Mar 9, 2023

@philip-cline I've reverted this back to using the previous approach of getting the deployed feed version with the feed source, so will work with the existing UI functionality. I'm still slightly concerned by how long this might take with 100+ feed sources. Please let me know how it performs!

Copy link
Contributor

@philip-cline philip-cline left a comment

Choose a reason for hiding this comment

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

This is looking good to me. It has sped things up for a large deployment to around 5s, which maybe isn't great but it's a start at least.

Copy link
Contributor

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

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

Working great! Just a few possibilities for speed improvements possibly, but I don't have any concrete suggestions so will approve while putting these ideas out there. Let me know what you think!

*/
@JsonIgnore
@BsonIgnore
private boolean deployedFeedVersionDefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we do this instead of a null check on deployedFeedVersion?

Copy link
Contributor Author

@br648 br648 Mar 15, 2023

Choose a reason for hiding this comment

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

This is to prevent calls to retrieveDeployedFeedVersion() where there are no deployed feed versions. Using null resulted in the following scenerio: Call to getDeployedFeedVersionId() returned null, followed by a call to getDeployedFeedVersionStartDate() returning null and a call to getDeployedFeedVersionEndDate() also returning null.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this may just be issues with my java understanding how is this field better than those 3 methods returning null? Is it to avoid mongo queries?

@br648
Copy link
Contributor Author

br648 commented Mar 16, 2023

Going to merge this to get the correct functionality to prod. Then look to include the project DB call into the two Mongo queries to further improve performance in a new PR.

@br648 br648 merged commit f1167b7 into dev Mar 16, 2023
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.

3 participants