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

Return otpServers with /project endpoint to allow speedup #514

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

philip-cline
Copy link
Contributor

@philip-cline philip-cline commented Feb 17, 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

Recent front end PRs (ibi-group/datatools-ui#895) have tried to reduce the amount of data that is unnecessarily requested by the front end. This endpoint created a conflict bc the /project and project:id differed in their return structure (specifically in returning otpServers).

Per the comment in the backend, I'm worried about performance issues with this change:
image

TODO: perhaps the ProjectWithOtpServers should not be a mixin anymore?

Copy link
Contributor

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

LGTM

@br648
Copy link
Contributor

br648 commented Feb 24, 2023

@philip-cline following on from your comment, have you noticed any performance issues with this change and is this related to your TODO? I.e. if there is a performance issue, should this update be rolled back?

@br648 br648 assigned philip-cline and unassigned br648 Feb 24, 2023
@philip-cline
Copy link
Contributor Author

Let me do some testing on a large project deployment and see. The TODO is separate from the performance issue, that is more just for if this fix is permanent, then it doesn't really make sense to keep the field as a mixin.

@philip-cline
Copy link
Contributor Author

Yeah this seems to slow things down significantly for a big project. I'll do some work to maybe reduce the amount of data we need to request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants