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

Fixes #1556 adding Django env #1557

Conversation

MFlyer
Copy link
Member

@MFlyer MFlyer commented Nov 30, 2016

#1556 :

To @schakrava : here my PR, hope had it right way (actually Pincardmanager working fine!)

Ready to review
Mirko

Signed-off-by: Mirko Arena <mirko.arena@gmail.com>
@schakrava
Copy link
Member

Hi @MFlyer this pr made me realize there's a design flaw in the pincard manager implementation. Let me explain. data-collector is a separate service, and we could use django-ORM to directly update models in the database. You are doing this now, but it creates new postgresql connections and in my experience leads to bad behaviour with resource starvation and becomes hard to debug.

The preferred way is to expose CRUD operations on Pincard model via REST API just like we do it on other resource like Disk, Pool, User etc.. Then, data-collector would use APIWrapper.api_call() to create, update and delete Pincard models. Routing it this way is better than playing hard and fast with ORM. We do this in data-collector already for other operations like updating rockons metadata etc..

Using ORM directly from data-collector just for reads is usually OK, something to keep in mind as well if you just want to read something from db and there's no API...

@MFlyer
Copy link
Member Author

MFlyer commented Nov 30, 2016

Thanks @schakrava ,
will work on this asap (asap = before you close for 3.8.16 ;) )

EDIT to @schakrava about 3.8.16:
what are your plans with 3.8.16?
Let me explain: actually at work it's a "Keep calm and don't kill anyone" period at least till weekend, so I can code new Pincard view etc these days, but probably this would mean have it in the "wrong" way.
If we're having 3.8.16 soon, considering PincardManager hasn't daily running tasks (users supposed to create a pincard one time and to recover password only in case of emergency) like dashboard widgets, if you agree we can have it solved with this PR to grant a working release and queue CRUD way to 3.8.17 cycle.
Let me know and obviously I'll anyway try to have it asap :)

@schakrava
Copy link
Member

Hi @MFlyer . Please don't rush on the proper implementation. Do it thoroughly like you normally do. I am planning to close out 3.8.16 soon as I and @priyaganti finish testing and fix any important bugs we encounter in the process. So no clear estimate, but it would be great to get the release out within a week.

I am totally fine with merging this PR for 3.8.16 and replacing it with improvements after that. Sound good?

@MFlyer
Copy link
Member Author

MFlyer commented Dec 1, 2016

Thanks @schakrava ,
thinking about Pincard via APIWrapper not sure still deciding if have it with CBV of FBV (Pincard management is alike Logs Manager, more running of custom functions then models interactions)
M.

@schakrava
Copy link
Member

Looks like this change is required regardless of our design consideration, because of a django refactor since 1.7. check this out: https://docs.djangoproject.com/en/1.10/releases/1.7/#app-loading-refactor

@schakrava schakrava merged commit c278243 into rockstor:master Dec 4, 2016
@MFlyer MFlyer deleted the issue#1556_Missing_Django_models_on_data_collector branch December 4, 2016 10:35
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.

2 participants