[GitHub] drill pull request #1197: DRILL-6279: UI indicates operators that spilled in...

2018-04-07 Thread asfgit
Github user asfgit closed the pull request at:

https://github.com/apache/drill/pull/1197


---


[GitHub] drill pull request #1197: DRILL-6279: UI indicates operators that spilled in...

2018-04-05 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1197#discussion_r179557189
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/OperatorWrapper.java
 ---
@@ -190,8 +223,59 @@ public void addSummary(TableBuilder tb, 
HashMap majorFragmentBusyT
 tb.appendFormattedInteger(recordSum);
 
 final ImmutablePair peakMem = 
Collections.max(opList, Comparators.operatorPeakMemory);
-tb.appendBytes(Math.round(memSum / size));
-tb.appendBytes(peakMem.getLeft().getPeakLocalMemoryAllocated());
+
+//Inject spill-to-disk attributes
+Map avgSpillMap = null;
+Map maxSpillMap = null;
+if (hasSpilledToDisk) {
+  avgSpillMap = new HashMap<>();
+  //Average SpillCycle
+  float avgSpillCycle = (float) spillCycleSum/size;
--- End diff --

My bad. I was thinking the sum was a long. I'll fix this.


---


[GitHub] drill pull request #1197: DRILL-6279: UI indicates operators that spilled in...

2018-04-05 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1197#discussion_r179552510
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/OperatorWrapper.java
 ---
@@ -38,7 +39,16 @@
  * Wrapper class for profiles of ALL operator instances of the same 
operator type within a major fragment.
  */
 public class OperatorWrapper {
+  //private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(OperatorWrapper.class);
--- End diff --

Ok. I see a lot of other places where it is commented out. I'll uncomment 
it with an unused tag for the compler to ignore. Comes very handy in debugging 
when making changes


---


[GitHub] drill pull request #1197: DRILL-6279: UI indicates operators that spilled in...

2018-04-05 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1197#discussion_r179552379
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/OperatorWrapper.java
 ---
@@ -38,7 +39,16 @@
  * Wrapper class for profiles of ALL operator instances of the same 
operator type within a major fragment.
  */
 public class OperatorWrapper {
+  //private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(OperatorWrapper.class);
+
+  private static final String HTML_ATTRIB_SPILLS = "spills";
+  private static final String HTML_ATTRIB_CLASS = "class";
+  private static final String HTML_ATTRIB_STYLE = "style";
+  private static final String HTML_ATTRIB_TITLE = "title";
+  private static final DecimalFormat DECIMAL_FORMATTER = new 
DecimalFormat("#.##");
   private static final String UNKNOWN_OPERATOR = "UNKNOWN_OPERATOR";
+  //-ve value to indicate absence of metric
--- End diff --

Using a negative ("-ve") value constant. Will fix the comment


---


[GitHub] drill pull request #1197: DRILL-6279: UI indicates operators that spilled in...

2018-04-05 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1197#discussion_r179552086
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/OperatorWrapper.java
 ---
@@ -190,8 +223,59 @@ public void addSummary(TableBuilder tb, 
HashMap majorFragmentBusyT
 tb.appendFormattedInteger(recordSum);
 
 final ImmutablePair peakMem = 
Collections.max(opList, Comparators.operatorPeakMemory);
-tb.appendBytes(Math.round(memSum / size));
-tb.appendBytes(peakMem.getLeft().getPeakLocalMemoryAllocated());
+
+//Inject spill-to-disk attributes
+Map avgSpillMap = null;
+Map maxSpillMap = null;
+if (hasSpilledToDisk) {
+  avgSpillMap = new HashMap<>();
+  //Average SpillCycle
+  float avgSpillCycle = (float) spillCycleSum/size;
--- End diff --

Needed the value to be in decimal (formatter will limit it to 2 decimal 
spaces).


---


[GitHub] drill pull request #1197: DRILL-6279: UI indicates operators that spilled in...

2018-04-05 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1197#discussion_r179551936
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/OperatorWrapper.java
 ---
@@ -190,8 +223,59 @@ public void addSummary(TableBuilder tb, 
HashMap majorFragmentBusyT
 tb.appendFormattedInteger(recordSum);
 
 final ImmutablePair peakMem = 
Collections.max(opList, Comparators.operatorPeakMemory);
-tb.appendBytes(Math.round(memSum / size));
-tb.appendBytes(peakMem.getLeft().getPeakLocalMemoryAllocated());
+
+//Inject spill-to-disk attributes
+Map avgSpillMap = null;
+Map maxSpillMap = null;
+if (hasSpilledToDisk) {
+  avgSpillMap = new HashMap<>();
+  //Average SpillCycle
+  float avgSpillCycle = (float) spillCycleSum/size;
+  avgSpillMap.put(HTML_ATTRIB_TITLE, 
DECIMAL_FORMATTER.format(avgSpillCycle) + " Spills on average");
--- End diff --

Agreed. Will fix it.


---


[GitHub] drill pull request #1197: DRILL-6279: UI indicates operators that spilled in...

2018-04-05 Thread kkhatua
Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1197#discussion_r179551687
  
--- Diff: exec/java-exec/src/main/resources/rest/profile/profile.ftl ---
@@ -372,6 +372,16 @@ table.sortable thead .sorting_desc { background-image: 
url("/static/img/black-de

 
 

[GitHub] drill pull request #1197: DRILL-6279: UI indicates operators that spilled in...

2018-04-05 Thread arina-ielchiieva
Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1197#discussion_r179406094
  
--- Diff: exec/java-exec/src/main/resources/rest/profile/profile.ftl ---
@@ -372,6 +372,16 @@ table.sortable thead .sorting_desc { background-image: 
url("/static/img/black-de

 
 

[GitHub] drill pull request #1197: DRILL-6279: UI indicates operators that spilled in...

2018-04-05 Thread arina-ielchiieva
Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1197#discussion_r179403081
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/OperatorWrapper.java
 ---
@@ -38,7 +39,16 @@
  * Wrapper class for profiles of ALL operator instances of the same 
operator type within a major fragment.
  */
 public class OperatorWrapper {
+  //private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(OperatorWrapper.class);
+
+  private static final String HTML_ATTRIB_SPILLS = "spills";
+  private static final String HTML_ATTRIB_CLASS = "class";
+  private static final String HTML_ATTRIB_STYLE = "style";
+  private static final String HTML_ATTRIB_TITLE = "title";
+  private static final DecimalFormat DECIMAL_FORMATTER = new 
DecimalFormat("#.##");
   private static final String UNKNOWN_OPERATOR = "UNKNOWN_OPERATOR";
+  //-ve value to indicate absence of metric
--- End diff --

`-ve value`?


---


[GitHub] drill pull request #1197: DRILL-6279: UI indicates operators that spilled in...

2018-04-05 Thread arina-ielchiieva
Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1197#discussion_r179405507
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/OperatorWrapper.java
 ---
@@ -190,8 +223,59 @@ public void addSummary(TableBuilder tb, 
HashMap majorFragmentBusyT
 tb.appendFormattedInteger(recordSum);
 
 final ImmutablePair peakMem = 
Collections.max(opList, Comparators.operatorPeakMemory);
-tb.appendBytes(Math.round(memSum / size));
-tb.appendBytes(peakMem.getLeft().getPeakLocalMemoryAllocated());
+
+//Inject spill-to-disk attributes
+Map avgSpillMap = null;
+Map maxSpillMap = null;
+if (hasSpilledToDisk) {
+  avgSpillMap = new HashMap<>();
+  //Average SpillCycle
+  float avgSpillCycle = (float) spillCycleSum/size;
--- End diff --

Why float? You are dividing double by int.


---


[GitHub] drill pull request #1197: DRILL-6279: UI indicates operators that spilled in...

2018-04-05 Thread arina-ielchiieva
Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1197#discussion_r179405848
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/OperatorWrapper.java
 ---
@@ -190,8 +223,59 @@ public void addSummary(TableBuilder tb, 
HashMap majorFragmentBusyT
 tb.appendFormattedInteger(recordSum);
 
 final ImmutablePair peakMem = 
Collections.max(opList, Comparators.operatorPeakMemory);
-tb.appendBytes(Math.round(memSum / size));
-tb.appendBytes(peakMem.getLeft().getPeakLocalMemoryAllocated());
+
+//Inject spill-to-disk attributes
+Map avgSpillMap = null;
+Map maxSpillMap = null;
+if (hasSpilledToDisk) {
+  avgSpillMap = new HashMap<>();
+  //Average SpillCycle
+  float avgSpillCycle = (float) spillCycleSum/size;
+  avgSpillMap.put(HTML_ATTRIB_TITLE, 
DECIMAL_FORMATTER.format(avgSpillCycle) + " Spills on average");
--- End diff --

I think `s` can be in lower case: `spills on average`.


---


[GitHub] drill pull request #1197: DRILL-6279: UI indicates operators that spilled in...

2018-04-05 Thread arina-ielchiieva
Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1197#discussion_r179402877
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/OperatorWrapper.java
 ---
@@ -38,7 +39,16 @@
  * Wrapper class for profiles of ALL operator instances of the same 
operator type within a major fragment.
  */
 public class OperatorWrapper {
+  //private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(OperatorWrapper.class);
--- End diff --

If logger is not needed, remote it.


---


[GitHub] drill pull request #1197: DRILL-6279: UI indicates operators that spilled in...

2018-04-05 Thread arina-ielchiieva
Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1197#discussion_r179403811
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/OperatorWrapper.java
 ---
@@ -190,8 +223,59 @@ public void addSummary(TableBuilder tb, 
HashMap majorFragmentBusyT
 tb.appendFormattedInteger(recordSum);
 
 final ImmutablePair peakMem = 
Collections.max(opList, Comparators.operatorPeakMemory);
-tb.appendBytes(Math.round(memSum / size));
-tb.appendBytes(peakMem.getLeft().getPeakLocalMemoryAllocated());
+
+//Inject spill-to-disk attributes
+Map avgSpillMap = null;
+Map maxSpillMap = null;
+if (hasSpilledToDisk) {
+  avgSpillMap = new HashMap<>();
+  //Average SpillCycle
+  float avgSpillCycle = (float) spillCycleSum/size;
+  avgSpillMap.put(HTML_ATTRIB_TITLE, 
DECIMAL_FORMATTER.format(avgSpillCycle) + " Spills on average");
+  avgSpillMap.put(HTML_ATTRIB_STYLE, "cursor:help;" + spillCycleMax);
+  avgSpillMap.put(HTML_ATTRIB_CLASS, "spill-tag"); //JScript will 
inject Icon
+  avgSpillMap.put(HTML_ATTRIB_SPILLS, 
DECIMAL_FORMATTER.format(avgSpillCycle)); //JScript will inject Count
+  maxSpillMap = new HashMap<>();
+  maxSpillMap.put(HTML_ATTRIB_TITLE, "Most # Spills: " + 
spillCycleMax);
+  maxSpillMap.put(HTML_ATTRIB_STYLE, "cursor:help;" + spillCycleMax);
+  maxSpillMap.put(HTML_ATTRIB_CLASS, "spill-tag"); //JScript will 
inject Icon
+  maxSpillMap.put(HTML_ATTRIB_SPILLS, String.valueOf(spillCycleMax)); 
//JScript will inject Count
+}
+
+tb.appendBytes(Math.round(memSum / size), avgSpillMap);
+tb.appendBytes(peakMem.getLeft().getPeakLocalMemoryAllocated(), 
maxSpillMap);
+  }
+
+  /**
+   * Returns index of Spill Count/Cycle metric
+   * @param operatorType
+   * @return index of spill metric
+   */
+  private int getSpillCycleMetricIndex(CoreOperatorType operatorType) {
+String metricName = null;
--- End diff --

Initializing with null is redundant.


---


[GitHub] drill pull request #1197: DRILL-6279: UI indicates operators that spilled in...

2018-03-30 Thread kkhatua
GitHub user kkhatua opened a pull request:

https://github.com/apache/drill/pull/1197

DRILL-6279: UI indicates operators that spilled in-memory data to disk

1. Detect the presence of an operator that has spilled to disk, in the 
Operators Overview section of a query's profile page.
2. Introduced API to inject html attributes into a cell. This is used to 
inject details about the spill in the avg peak and max peak memory usage
3. Javascript is leveraged to detect spilled operators' HTML elements, and 
inject a symbolic font for indicating the operator as having spilt.
4. Mouse-over effect indicates the average and max number of spill cycles 
the operator went through. 

NOTE: Bootstrap library comes with Glyphicons Halfling Fonts and meets the 
Apache license requirements.
http://glyphicons.com/license#halflingsbootstrap

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/kkhatua/drill DRILL-6279

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/1197.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1197


commit 230d739c05e2f2da93232fe31dec55adf627937e
Author: Kunal Khatua 
Date:   2018-03-30T04:58:53Z

DRILL-6279: UI indicates operators that spilled in-memory data to disk

1. Detect the presence of an operator that has spilled to disk, in the 
Operators Overview section of a query's profile page.
2. Introduced API to inject html attributes into a cell. This is used to 
inject details about the spill in the avg peak and max peak memory usage
3. Javascript is leveraged to detect spilled operators' HTML elements, and 
inject a symbolic font for indicating the operator as having spilt.
4. Mouse-over effect indicates the average and max number of spill cycles 
the operator went through.

commit 5641e713888db6e3e028a5749bd902dc3bdc6562
Author: Kunal Khatua 
Date:   2018-03-26T05:05:06Z

Addition of Bootstrap's Glyphicons

As part of the Bootstrap's components, this meets Apache License criteria




---