-
Notifications
You must be signed in to change notification settings - Fork 73
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
Fixes #9857: ReportingServiceImpl moves around a lot of data and is fairly slow #1409
Conversation
@@ -267,6 +267,9 @@ class HomePage extends Loggable { | |||
|
|||
val globalCompliance = compliance.complianceWithoutPending.round | |||
|
|||
val n4 = System.currentTimeMillis | |||
TimingDebugLogger.debug(s"Compute compliance for HomePage: ${n4 - n2}ms") |
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 had to move this log, as all compliance are lazy now
Commit modified |
d48a194
to
520e532
Compare
val compliance = report.compliance | ||
val byRules: Map[RuleId, AggregatedStatusReport] = report.reports.groupBy(_.ruleId).mapValues(AggregatedStatusReport(_)) | ||
lazy val compliance = report.compliance | ||
lazy val byRules: Map[RuleId, AggregatedStatusReport] = report.reports.groupBy(_.ruleId).mapValues(AggregatedStatusReport(_)) | ||
} | ||
|
||
object NodeStatusReport { | ||
def apply(nodeId: NodeId, runInfo: RunAndConfigInfo, statusInfo: RunComplianceInfo, reports: Iterable[RuleNodeStatusReport]) = { |
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.
Do we still use the old apply method ?
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.
yes, it is still used in ReportDisplayer
Commit modified |
520e532
to
a0b33db
Compare
@@ -680,7 +680,7 @@ object ExecutionBatch extends Loggable { | |||
} | |||
} | |||
|
|||
NodeStatusReport(nodeId, runInfo, status, ruleNodeStatusReports) | |||
NodeStatusReport.applyByNode(nodeId, runInfo, status, ruleNodeStatusReports) |
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 changed that, as we are sure it is from the current node
@@ -947,21 +947,17 @@ object ExecutionBatch extends Loggable { | |||
*/ | |||
private[reports] def checkExpectedComponentWithReports( | |||
expectedComponent: ComponentExpectedReport | |||
, filteredReports : Seq[Reports] | |||
, filteredReports : Seq[ResultReports] |
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.
by design, it is only ResultReports
This PR is not mergeable to upper versions. |
a0b33db
to
833beda
Compare
PR rebased |
This PR is not mergeable to upper versions. |
OK, merging this PR |
*/ | ||
def filterByRules(nodeStatusReport: NodeStatusReport, ruleIds: Set[RuleId]) : NodeStatusReport = { | ||
new NodeStatusReport( | ||
nodeStatusReport.forNode |
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.
what is this forNode ?
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.
ha, this is up there
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.
in 4.1 this is nodeId ...
OK, merging this PR |
https://www.rudder-project.org/redmine/issues/9857