date:20180301

[GitHub] drill issue #1141: DRILL-6197: Skip duplicate entry for OperatorStats

2018-03-01 Thread amansinha100

Github user amansinha100 commented on the issue:

https://github.com/apache/drill/pull/1141
  
+1.  


---

[GitHub] drill pull request #1141: DRILL-6197: Skip duplicate entry for OperatorStats

2018-03-01 Thread kkhatua

Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1141#discussion_r171767495
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentStats.java ---
@@ -79,4 +71,21 @@ public void addOperatorStats(OperatorStats stats) {
 operators.add(stats);
   }
 
+  //DRILL-6197
+  public OperatorStats addOrReplaceOperatorStats(OperatorStats stats) {
+//Remove existing stat
+OperatorStats replacedStat = null;
+int index = 0;
+for (OperatorStats opStat : operators) {
--- End diff --

Everything worked fine. Tried doing a join for TPCH tables - `lineitem` and 
`orders`, and confirmed no more duplicates for SCREEN, SINGLE_SENDER and 
HASH_PARTITION_SENDER. For a smaller substitute of `orders` with `supplier` ; 
confirmed that the BROADCAST_SENDER was also not having duplicates..


---

[GitHub] drill pull request #1141: DRILL-6197: Skip duplicate entry for OperatorStats

2018-03-01 Thread amansinha100

Github user amansinha100 commented on a diff in the pull request:

https://github.com/apache/drill/pull/1141#discussion_r171753536
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentStats.java ---
@@ -79,4 +71,21 @@ public void addOperatorStats(OperatorStats stats) {
 operators.add(stats);
   }
 
+  //DRILL-6197
+  public OperatorStats addOrReplaceOperatorStats(OperatorStats stats) {
+//Remove existing stat
+OperatorStats replacedStat = null;
+int index = 0;
+for (OperatorStats opStat : operators) {
--- End diff --

LGTM.  Hopefully it did not break existing stuff..so will wait for your 
confirmation.   


---

[GitHub] drill issue #1105: DRILL-6125: Fix possible memory leak when query is cancel...

2018-03-01 Thread ilooner

Github user ilooner commented on the issue:

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

@arina-ielchiieva @vrozov I believe I have a solution. There were several
issues with the original code.

1. It made incorrect assumptions about how cache invalidation works with
java **synchronized**.
2. It assumed **innerNext** and **close** would be called sequentially.

I believe this PR fixes these issues now and I have gone into more detail
about the problems below.

# 1. Incorrect Cache Invalidation Assumptions

The original code was trying to be smart by trying to reduce
synchronization overhead on **innerNext**. So the code in **innerNext** did not
synchronize before changing the partitioner object since this would be called
often. The code in **close()** and ** receivingFragmentFinished()**
synchronized before accessing the partitioner with the intention that this
would trigger an update of the partitioner variable state across all threads.
Unfortunately, this assumption is invalid (see
https://stackoverflow.com/questions/22706739/does-synchronized-guarantee-a-thread-will-see-the-latest-value-of-a-non-volatile).
Every thread that accesses a variable must synchronize before accessing a
variable in order to properly invalidate cached data on a core.

For example if **Thread A** modifies **Variable 1** then **Thread B**
synchronizes before accessing **Variable 1** then there is no guarantee
**Thread B** will see the most updated value for **Variable 1** since it might .

## Solution

In summary the right thing to do is the simple thing. Make the methods
synchronized. Unfortunately there is no way to outsmart the system and reduce
synchronization overhead without causing race conditions.

# 2. Concurrent InnerNext and Close Calls

The original code did not consider the case that innerNext was in the
middle of execution when close was called. It did try to handle the case where
**innerNext** could be called after **close** by setting the **ok** variable.
But it didn't even do that right because there was no synchronization around
the **ok** variable.

## Solution

The right thing to do is the simple thing. Make sure the methods are
synchronized so close has to wait until innerNext is done before executing.
Also when a query is cancelled the executing thread should be interrupted the
thread running innerNext incase it is on a blocking call.

57 matches

Mail list logo