Fixes #7735: OOM in Rudder when there are too many repaired reports #1084
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -478,6 +515,16 @@ class ReportsJdbcRepository(jdbcTemplate : JdbcTemplate) extends ReportsReposito | |||
| } | |||
| } | |||
|
|
|||
| final case class CoundChangesMapper(intMapper: Int => Interval) extends RowMapper[(RuleId, Int, Interval)] { | |||
There was a problem hiding this comment.
it should be CountChangesMapper
|
First commit is ok ! and is the easiest one, now let's take a look at the big ones ... |
|
PR updated |
fanf
force-pushed
the
bug_7735/oom_in_rudder_when_there_are_too_many_repaired_reports
branch
from
8357999
to
190a719
Compare
| * Get the changes for the given interval, without using the cache. | ||
| */ | ||
| override def getChangesForInterval(ruleId: RuleId, interval: Interval, limit: Option[Int]): Box[Seq[ResultRepairedReport]] = { | ||
| for { |
There was a problem hiding this comment.
Why not just 'changeService.getChangesForInterval(ruleId, interval, limit)' ?
There was a problem hiding this comment.
well, certainly there was some other comands at some point of the dev, that were removed in the end (but not the for/yield structure).
|
Apart the two minor fixes it's ok and can be merged (once it's one commit! ) |
|
PR updated |
fanf
force-pushed
the
bug_7735/oom_in_rudder_when_there_are_too_many_repaired_reports
branch
from
190a719
to
f4db618
Compare
fanf
deleted the
bug_7735/oom_in_rudder_when_there_are_too_many_repaired_reports
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://www.rudder-project.org/redmine/issues/7735
For information, with the 3 patches applied (+https://www.rudder-project.org/redmine/issues/8195), with > 1.2 Millions changes in the last 3 days (with one 6h interval at 1M changes), we are able to start a Rudder with -Xmx600m, have the non-compliant report be iteratively written, login, go to rule details, see graphes, click and see details on all interval, and everything is fast - with a top heap consomption at 290MB.
So, there is still some cleaning to do, but the general architecture of the patch is ok. It is in 3 parts, I'm letting them in three commits to make them better understandable. Perhaps it could be split in 3 tickets, to, but they are highly coupled (at least the last two), so not sure.
1/ Today, we reload compliance each time we switch from the Rule edit form tab to the compliance one. This is useless since we have nice "reload" button on compliance tab, it is an UX strangeness because it causes delay for switch, and don't let the user choose when data should change (difficult to understand, or crazy when you were trying to analized something and clicked on the tab by mistake), and it leads to OOM when there is a lot of reports (because even if we succeeded in calculating one time, the second one, we have much less memory, since a big part is used).
2/ That patches allows to only load the events details (display on the bottom grid) for the last interval in rule details (and load details of other on click on the corresponding graph interval). That also means that much less data are transfered to the browser, making user happier (end rule details faster to display). More over, a hard limit of max 10 000 event for a rule, for an interval, is introduced. It is not a good solution, but the correct one (pagination on each interval) is much harder to implement, with higher risks in a released version.
3/ With the third patch, the event cache only keep a COUNT of the number of events by rule by interval, which consume much less memory than keeping all events (of course). Also rewrote some other methods to consum less memory overall.