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

Suggesting some new methods #18

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

ulmitov
Copy link
Collaborator

@ulmitov ulmitov commented Aug 23, 2023

No description provided.

@ulmitov ulmitov requested a review from ralequi August 23, 2023 02:41
@ralequi ralequi self-assigned this Aug 24, 2023
@coveralls
Copy link

Coverage Status

coverage: 61.185% (-0.2%) from 61.34% when pulling c87f581 on ulmitov:new_methods into c9cfcec on Naudit:master.

else:
self._show = common.response_data_subkey(
out, ['System Overview', 'IT System Overview'])
return self._show
Copy link
Member

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

Copy link
Collaborator Author

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:)

  1. each time user should pay attention in code when to turn it on and off - code gets complicated

  2. 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.

  3. 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.

Copy link
Collaborator Author

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

Copy link
Member

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

Copy link
Collaborator Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

I should check that !

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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

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