Adding MemoryMonitor to enforce memory limits #805
Conversation
| this.event = event; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
How about "fire" method that handles calling the callback and changing state? You could also have a method to encapsulate checking its threshold
There was a problem hiding this comment.
Not sure I understand. You're proposing to add 2 more methods with the logic currently implemented in checkThresholdMonitors?
| for (ThresholdMonitor<T> m: monitors) { | ||
| if (dataSizeB > m.thresholdB && !m.fired) { | ||
| m.fired = true; | ||
| m.event.fire(dataSizeB, m.thresholdB, obj); |
There was a problem hiding this comment.
May be worth capturing a TODO or something here that the first monitor to throw an exception will stop all other callbacks from firing. As a general purpose class, this is probably surprising, though we know with its current usages it may not be problematic. So, I suggest at least a TODO and/or open an issue to remind ourselves that this should probably catch any thrown exceptions and aggregate them.
There was a problem hiding this comment.
Without a use case this does not justify a TODO/issue imo. I will clarify in javadoc.
The limits need to be enforced in multiple locations: Response, BulkExecutionContext, HookManager and IterateAndUpdate (last one from lightblue-mongo). To avoid logic redundancy, creating MemoryMonitor utility.
Note that Mediator level hooks are not used (#804).