Don't queue EmsRefresh if using streaming refresh #17531
Conversation
|
Hey @Fryguy one of the things that we still need to handle with streaming refresh is what to do about places that queue standard This will prevent new refreshes from being queued (e.g. all of the automate event handlers) and ManageIQ/manageiq-providers-vmware#284 will prevent existing queued refresh messages from being delivered. WDYT? |
| @@ -51,6 +51,9 @@ def self.queue_refresh(target, id = nil, opts = {}) | |||
| h[e] << t unless e.nil? | |||
| end | |||
|
|
|||
| # Drop targets on EMSs which are using streaming refresh | |||
| targets_by_ems.reject! { |ems, _| ems.supports_streaming_refresh? } | |||
There was a problem hiding this comment.
Some providers support it, but don't "currently" support it because of a setting flag...does this method handle that case?
There was a problem hiding this comment.
Yes we're checking the settings for if it is enabled not just if it is supported here
|
Just looked at the other PR: I would be wary about the support_* prefix because that is "reserved" by the supports_feature thing. But on that note, maybe this should be a supports_feature thing directly. That way, you can handle that "supports refresh but not right now" case. |
|
Yeah you're right I'll change it to use supports_feature. |
agrare
force-pushed
the
dont_queue_refreshes_for_streaming_refresh
branch
3 times, most recently
from
9b2e8af
to
bf660b6
Compare
|
@Fryguy okay updated |
agrare
force-pushed
the
dont_queue_refreshes_for_streaming_refresh
branch
5 times, most recently
from
96f2be9
to
f1aa6ba
Compare
|
Where is the default defined? Or is that part of support feature itself? |
|
@Fryguy all supported features are defaulted to false, https://github.com/ManageIQ/manageiq/blob/master/app/models/mixins/supports_feature_mixin.rb#L145-L150 |
agrare
force-pushed
the
dont_queue_refreshes_for_streaming_refresh
branch
from
f1aa6ba
to
0a740a4
Compare
There was a problem hiding this comment.
Awesome 👍
Full refresh on worker start is solved here:
https://github.com/ManageIQ/manageiq-providers-vmware/blob/master/app/models/manageiq/providers/vmware/infra_manager/inventory/collector.rb#L27
If a target's ems is using streaming refresh don't queue an EmsRefresh because it will not be processed.
agrare
force-pushed
the
dont_queue_refreshes_for_streaming_refresh
branch
from
0a740a4
to
946bd7f
Compare
|
Checked commit agrare@946bd7f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
If a target's ems is using streaming refresh don't queue an EmsRefresh
because it will not be processed.
VMware associated PR: ManageIQ/manageiq-providers-vmware#284