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
Better cleanup and handle limits #14
Conversation
Codecov Report
@@ Coverage Diff @@
## master #14 +/- ##
==========================================
- Coverage 78.16% 77.74% -0.42%
==========================================
Files 236 238 +2
Lines 18250 18420 +170
==========================================
+ Hits 14265 14321 +56
- Misses 3985 4099 +114
Continue to review full report at Codecov.
|
.then((dialog) => { | ||
this.alert = `Removing plugin ${plugin} ...` | ||
this.alertType = 'success' | ||
this.showAlert = true |
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.
Removing a plugin can take a while so I wanted to notify the user that it started
}) | ||
.then((limitsSubscription) => { | ||
this.limitsSubscription = limitsSubscription | ||
}) |
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.
Moving the subscription here is duplication of the Events subscription but greatly simplifies the design since the Events wants history
}, | ||
methods: { | ||
updateOutOfLimits() { | ||
this.items = [] | ||
this.itemList = [] |
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.
This method now initializes the visible list of items
@@ -307,13 +338,22 @@ export default { | |||
}, | |||
update() { | |||
if (this.$store.state.tlmViewerItems.length !== 0) { | |||
// localStorage.axiosIgnoreResponse = '500' // localStorage only supports strings |
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.
This would potentially prevent popups on bad items but I think my config events handler mostly covers this now. Left it just in case as it's a useful pattern.
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.
This is a global setting, so probably dangerous?
}, | ||
}, | ||
{ | ||
history_count: 1000, |
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.
History makes this incompatible with LimitsControl
@@ -82,6 +82,7 @@ def decom_packet(topic, _msg_id, msg_hash, _redis) | |||
# @param value [Object] The current value of the item | |||
# @param log_change [Boolean] Whether to log this limits change event | |||
def limits_change_callback(packet, item, old_limits_state, value, log_change) | |||
return if @cancel_thread |
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 added this during my early debugging but I think it's a small optimization that still makes sense
model_instance.destroy | ||
end | ||
# Wait for the operator to wake up and remove the microservice processes | ||
sleep 30 # Cycle time 15s plus 2s wait for soft stop and then hard stop |
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 didn't really know the best way to signal the MicroserviceOperator that it's time to remove processes so I just left the sleep. If you want to optimize from here that would be awesome.
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'm going to decrease the operator cycle time to 5s and reduce this.
%w(DECOM COMMANDLOG DECOMCMDLOG PACKETLOG DECOMLOG REDUCER CLEANUP).each do |type| | ||
model = MicroserviceModel.get_model(name: "#{@scope}__#{type}__#{@name}", scope: @scope) | ||
model.destroy if model | ||
end |
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 think this is unnecessary due to the code in plugin_model undeploy
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.
Not unnecessary from a standalone perspective. Needs to be added back.
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 will restore.
Store.hdel("#{scope}__current_limits", item) | ||
if item =~ /^#{target_name}__#{packet_name}__/ | ||
Store.hdel("#{scope}__current_limits", item) | ||
end |
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.
Previously we were just deleting everything? That's wrong so I delete based on matching the name.
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.
Good catch
%w(DECOM COMMANDLOG DECOMCMDLOG PACKETLOG DECOMLOG REDUCER CLEANUP).each do |type| | ||
model = MicroserviceModel.get_model(name: "#{@scope}__#{type}__#{@name}", scope: @scope) | ||
model.destroy if model | ||
end |
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.
Not unnecessary from a standalone perspective. Needs to be added back.
Store.hdel("#{scope}__current_limits", item) | ||
if item =~ /^#{target_name}__#{packet_name}__/ | ||
Store.hdel("#{scope}__current_limits", item) | ||
end |
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.
Good catch
@@ -307,13 +338,22 @@ export default { | |||
}, | |||
update() { | |||
if (this.$store.state.tlmViewerItems.length !== 0) { | |||
// localStorage.axiosIgnoreResponse = '500' // localStorage only supports strings |
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.
This is a global setting, so probably dangerous?
model_instance.destroy | ||
end | ||
# Wait for the operator to wake up and remove the microservice processes | ||
sleep 30 # Cycle time 15s plus 2s wait for soft stop and then hard stop |
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'm going to decrease the operator cycle time to 5s and reduce this.
%w(DECOM COMMANDLOG DECOMCMDLOG PACKETLOG DECOMLOG REDUCER CLEANUP).each do |type| | ||
model = MicroserviceModel.get_model(name: "#{@scope}__#{type}__#{@name}", scope: @scope) | ||
model.destroy if model | ||
end |
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 will restore.
closes #2