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

feat: make plugin refresh interval configurable #360

Merged
merged 1 commit into from
Feb 5, 2020

Conversation

edaniszewski
Copy link
Contributor

This PR:

  • makes the interval for the plugin refresh task configurable

fixes #358

@edaniszewski edaniszewski linked an issue Feb 5, 2020 that may be closed by this pull request
Copy link
Contributor

@MatthewHink MatthewHink left a comment

Choose a reason for hiding this comment

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

Okay the refresh_every

As the configuration becomes more difficult we should doc (comment) this in the deploy files.

So many knobs, levers, tweaks can become unwieldy. Usually the less the better.

@MatthewHink
Copy link
Contributor

I am strongly against this because it feels like it's masking the real bug.

The i2c plugins came up inactive on a deploy today, though normally they come up just fine.

Why does this help? - And thanks for merging.

-1.

@edaniszewski
Copy link
Contributor Author

I saw the approve but the page didn't refresh so I didn't see the comment you left - my bad.

While I agree that having excessive amounts of configuration can be overwhelming and can then make an application feel fragile as it becomes an exercise in tinkering with configs, I do feel like some values need to be configurable for an application to be generally usable. It may be premature to add this now since we are currently the only ones using it, but if there ever are other users, their use case may be different from ours.

I opened an issue in the synse-docs repo to capture this change, so the configuration fields and values will be documented with examples, so even though it increases config complexity, it is navigable with docs.

As for this masking an issue of plugins coming up inactive - I don't think it does that. It just makes a value configurable. We could abuse this value to mask the issue, but we could also leverage it to adjust runtime conditions to try to replicate the actual issue and resolve it.

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.

make plugin refresh task period configurable
3 participants