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

Add docs on performance optimizations. #104

Merged
merged 10 commits into from
May 3, 2023
Merged

Add docs on performance optimizations. #104

merged 10 commits into from
May 3, 2023

Conversation

Kircheneer
Copy link
Contributor

@Kircheneer Kircheneer commented Apr 26, 2023

Closes #98

@Kircheneer Kircheneer requested a review from a team as a code owner April 26, 2023 14:14
Copy link
Contributor

@jdrew82 jdrew82 left a comment

Choose a reason for hiding this comment

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

LGTM!

Co-authored-by: Ken Celenza <ken@celenza.org>
Copy link
Contributor

@chadell chadell left a comment

Choose a reason for hiding this comment

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

I don't see any mention of the atomic transaction topic, and how to leverage post_run.
Also, I think it could be interesting to introduce the trade-off of using bulk operations

docs/user/developing_jobs.md Outdated Show resolved Hide resolved
docs/user/developing_jobs.md Outdated Show resolved Hide resolved
Co-authored-by: Christian Adell <chadell@gmail.com>
@Kircheneer
Copy link
Contributor Author

I don't see any mention of the atomic transaction topic, and how to leverage post_run. Also, I think it could be interesting to introduce the trade-off of using bulk operations

I wanted to (for now) stick to things for which I concretely know that they don't pose any negatives, with post_run losing the atomic transaction and resulting in abnormal job behavior (you have to manually reload the job result, it shows as completed too early) and bulk operations not running custom model validation or even save methods.

These optimizations could be documented at a later point in time IMO.

Copy link
Contributor

@chadell chadell left a comment

Choose a reason for hiding this comment

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

Add .idea to gitignore

@chadell
Copy link
Contributor

chadell commented Apr 27, 2023

Yes, they can be added in another PR, but they are in the scope of #98 issue

@Kircheneer
Copy link
Contributor Author

I have responded to most the requests now. Open points:

Using bulk_create

I don't feel good recommending this in the documentation as is right now because of its severe trade-offs:

I argue that we need explicit Nautobot core support for this to be reasonable.

Parallelizing the load functions

I think this needs to be supported from SSoT itself rather than a recommendation in the docs for people to build it themselves. To me it only spells trouble when users would have to convert their custom methods of parallelization to a method that eventually will be supported out of this app.

@chadell thoughts?

@chadell
Copy link
Contributor

chadell commented May 3, 2023

on't feel good recommending this in the documentation as is right now because of its severe trade-offs:

I argue that we need explicit Nautobot core support for this to be reasonable.

  1. If we do not document it (even for discouraging), someone else will ask the question again. I prefer to be explicit about it, so everyone can make their own decisions. I mean, documenting != recommending.
  2. Parallelization of load jobs should be out of the scope, as it will be tackled after Nautobot 2.0 release. The other question is parallelizing a single job with concurrent API operations. Again, I am not saying we recommend doing it, but explaining potential pros/cons will make everyone understand it.

@Kircheneer
Copy link
Contributor Author

All feedback addressed

Copy link
Contributor

@chadell chadell left a comment

Choose a reason for hiding this comment

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

LGTM

@Kircheneer Kircheneer merged commit 6f82d8c into nautobot:develop May 3, 2023
16 checks passed
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.

Document how to efficiently load data from Nautobot
5 participants