-
Notifications
You must be signed in to change notification settings - Fork 7
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
Suggesting some new methods #18
base: master
Are you sure you want to change the base?
Conversation
else: | ||
self._show = common.response_data_subkey( | ||
out, ['System Overview', 'IT System Overview']) | ||
return self._show |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is exactly the point of this?
The cache is used to avoid duplicated calls, and this seems to be a custom cache over a "global" cache.
Don't see the point but another issue-maker to have multiple versions of the same "show".
Have you found an issue with the cache? Maybe that's what we must address instead of this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, i have a list of issues:)
-
each time user should pay attention in code when to turn it on and off - code gets complicated
-
if not storing some basic info somewhere, run logs become huge, since for each property the command is invoked from scratch. But many properties are static, will not change, so those calls can be saved.
-
if using singleton, then user should address only one pystorcli object to enable/disable the global cache
but if not using singleton, then user should first find the exact pystrocli object where he wants to enable/disable cache, and it can be multiple objects - for example all drives of a VD.
4.And specifically regarding singleton: suppose host has two raid/hba cards of different models that also use different cli versions (did not see it yet with broadcom but saw it with adaptecs), so user wants to have now two different storcli versions - now he can't use singleton option and global caching also becomes complicated.
So basically the global cache approach is not really needed, because for static properties they can be stored somewhere once and for all other properties which are dynamic - why would some one use caching for dynamic things, it will be a wrong code flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this would be also relevant for #14
Would be great to dispose the singleton and global cache, the runner should simple be the same one which pysmart uses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw cache on pysmart... and I think it really improves performance without much coding effort.
In the other hand, I see your point. There are scenarios where singleton+global configs/cache can be harder than initially expected.
Maybe a cache-per-property may be much better (probably using custom decorator? and/or some external cache-lib decorator?)
I have to think about this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference with pysmart is that many objects can change dynamically - creating/deleting vds, switching modes, switching drive states, etc..
A cache-per-property would be great, that is sort of what i tried to do with a static show property.
In pyarcconf i did it differently - most of objects and properties are always being cached, but each object has an update method. So when doing an action which might change the object, after that it will call for its update. Sort of a cache per object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should check that !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tell me later if you are ok with the static show prop, maybe as a temp solution, because i want to also add a show_all prop for drive object, most of runs use a lot of the drive props
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correction, in pyrcconf only the "show all" command is being cached, the update is refreshing the "show all", all the rest is fetched on the fly
No description provided.