-
Notifications
You must be signed in to change notification settings - Fork 18
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
Xdmod9.0/appkernel data aggregation for hichart #76
Xdmod9.0/appkernel data aggregation for hichart #76
Conversation
…hich lead to empty report
…pkernel db use from sql query
…plicit database naming in sql quories
# Conflicts: # bin/appkernel_reports_manager # classes/AppKernel/PerformanceMap.php
…ppkernels into xdmod9.0/akrr2.0_update
…5/akrr2.0 # Conflicts: # classes/AppKernel/PerformanceMap.php # classes/OpenXdmod/Setup/AppKernelsSetup.php
…ion-for-hichart # Conflicts: # xdmod-appkernels.spec.in
…ial name for display, nickname for internal use)
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.
Hi Nikolay,
Here's a first go at a review on this PR, there is quite a lot of comments but on the plus side it's really just a few common things that can just be blanket updated.
re: the parameterized query thing, I can definitely help out this wherever you might need it.
Thanks!
classes/AppKernel/AppKernelDb.php
Outdated
if($Nallupdate_metric_data>=500||$i >= $length-1) {// | ||
$time_start_sqlupdate = microtime(true); | ||
$collected_array=implode(',', $collected_array); | ||
$allupdate_metric_data="UPDATE metric_data SET |
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.
parameterize query.
classes/AppKernel/AppKernelDb.php
Outdated
$time_end_sqlupdate1 = microtime(true); | ||
$timing['sqlupdate1']+=$time_end_sqlupdate1 - $time_start_sqlupdate; | ||
|
||
$allupdate_a_data2="UPDATE a_data2 SET |
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.
parameterize query.
classes/AppKernel/AppKernelDb.php
Outdated
|
||
// ak_instance | ||
if($metrics_walltime_id!==null && $dataset['metric_id']===$metrics_walltime_id) { | ||
$allupdate_ak_instance="UPDATE ak_instance SET |
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.
parameterize query.
@@ -0,0 +1,19 @@ | |||
{ |
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 don't think this file should be included as it's only added by the style checker.
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.
it should be there or eslint should complain on unknown namespaces and travis will fail styling checking
@@ -9,13 +9,15 @@ | |||
*/ |
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.
A comment for this whole class, any variables whose names are are less then 3 characters need to be updated to be descriptive of what they are referring to / what data they contain.
Just a quick question, do you have an editor that you prefer using? A number of them support setting up a set of code format guidelines that you can then have automatically applied to whatever bit of code that you're working on. I think that would make the vast majority of the comments ( minus the parameterized query ones ) just a single key stroke per file to update. |
Surely I use good editor, but I was assuming php code sniffer with settings from xdmod-qa provides enough coverage for our coding standard and use it for linting. On the other hand this is largely issue solving PR with a fair amount of styling fixes. |
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.
Looks good, you'll just want to make Travis happy but after that it should be good to go.
Added data aggregation for plotting to overcome hichart limitations on large datasets.
Was:




Now:
Was:
Now:
There are also some conversions to handle AKRR 2.0