Re: Review Request 53166: HIVE-14910: Flaky test: TestSparkClient.testJobSubmission

[Barna Zsombor Klara]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53166/
---

(Updated Nov. 2, 2016, 9:35 a.m.)


Review request for hive, Mohit Sabharwal, Siddharth Seth, and Xuefu Zhang.


Changes
---

Renamed the variables in JobHandleImp as requested in the review.


Repository: hive-git


Description
---

HIVE-14910: Flaky test: TestSparkClient.testJobSubmission
I ran into this problem today while investigating a flaky test. I think the 
failure is coming from this race condition: the listener can be added to the 
JobHandle only after the job has been submitted. So there is no guarantee that 
every method of the listener will be invoked, some state changes may have 
happened before the caller received the handler back.
I propose a slight change in the API. We should add the listeners as an 
argument of the submit method, so we can set them on the Handler before the job 
itself is submitted. This way any status change should be signalled to the 
listener.


Diffs (updated)
-

  spark-client/src/main/java/org/apache/hive/spark/client/JobHandle.java 
44aa255a8271894ed3e787c3e7d1323628db63c4 
  spark-client/src/main/java/org/apache/hive/spark/client/JobHandleImpl.java 
17c8f40edd472682d5604f41980d06e60cc92893 
  spark-client/src/main/java/org/apache/hive/spark/client/SparkClient.java 
3e921a5d9b77966d368684ee7b6f1c861ac60e08 
  spark-client/src/main/java/org/apache/hive/spark/client/SparkClientImpl.java 
e2a30a76e0f7fe95d8a453f502311baa08abcbe2 
  spark-client/src/test/java/org/apache/hive/spark/client/TestJobHandle.java 
e8f352dce9f618573c2d79e9b8c59e19fad7298a 
  spark-client/src/test/java/org/apache/hive/spark/client/TestSparkClient.java 
b95cd7a05d44224b53bf2cef9170146b8b2eb4a8 

Diff: https://reviews.apache.org/r/53166/diff/


Testing
---

Unit tests modified and tested.
I also ran a simple query with HoS as the execution engine.


Thanks,

Barna Zsombor Klara

Re: Review Request 53166: HIVE-14910: Flaky test: TestSparkClient.testJobSubmission

[Xuefu Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53166/#review154132
---




spark-client/src/main/java/org/apache/hive/spark/client/JobHandleImpl.java 
(line 165)


Nit: could we avoid using single letter variable name for things other than 
integer iterators? Same below.


- Xuefu Zhang


On Oct. 26, 2016, 1:48 p.m., Barna Zsombor Klara wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53166/
> ---
> 
> (Updated Oct. 26, 2016, 1:48 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal, Siddharth Seth, and Xuefu Zhang.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-14910: Flaky test: TestSparkClient.testJobSubmission
> I ran into this problem today while investigating a flaky test. I think the 
> failure is coming from this race condition: the listener can be added to the 
> JobHandle only after the job has been submitted. So there is no guarantee 
> that every method of the listener will be invoked, some state changes may 
> have happened before the caller received the handler back.
> I propose a slight change in the API. We should add the listeners as an 
> argument of the submit method, so we can set them on the Handler before the 
> job itself is submitted. This way any status change should be signalled to 
> the listener.
> 
> 
> Diffs
> -
> 
>   spark-client/src/main/java/org/apache/hive/spark/client/JobHandle.java 
> 44aa255a8271894ed3e787c3e7d1323628db63c4 
>   spark-client/src/main/java/org/apache/hive/spark/client/JobHandleImpl.java 
> 17c8f40edd472682d5604f41980d06e60cc92893 
>   spark-client/src/main/java/org/apache/hive/spark/client/SparkClient.java 
> 3e921a5d9b77966d368684ee7b6f1c861ac60e08 
>   
> spark-client/src/main/java/org/apache/hive/spark/client/SparkClientImpl.java 
> e2a30a76e0f7fe95d8a453f502311baa08abcbe2 
>   spark-client/src/test/java/org/apache/hive/spark/client/TestJobHandle.java 
> e8f352dce9f618573c2d79e9b8c59e19fad7298a 
>   
> spark-client/src/test/java/org/apache/hive/spark/client/TestSparkClient.java 
> b95cd7a05d44224b53bf2cef9170146b8b2eb4a8 
> 
> Diff: https://reviews.apache.org/r/53166/diff/
> 
> 
> Testing
> ---
> 
> Unit tests modified and tested.
> I also ran a simple query with HoS as the execution engine.
> 
> 
> Thanks,
> 
> Barna Zsombor Klara
> 
>

Re: Review Request 53166: HIVE-14910: Flaky test: TestSparkClient.testJobSubmission

[Barna Zsombor Klara]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53166/
---

(Updated Oct. 26, 2016, 1:48 p.m.)


Review request for hive, Mohit Sabharwal, Siddharth Seth, and Xuefu Zhang.


Changes
---

Removed unused import and fixed minor bug in the constructor of JobHandleImpl.


Repository: hive-git


Description
---

HIVE-14910: Flaky test: TestSparkClient.testJobSubmission
I ran into this problem today while investigating a flaky test. I think the 
failure is coming from this race condition: the listener can be added to the 
JobHandle only after the job has been submitted. So there is no guarantee that 
every method of the listener will be invoked, some state changes may have 
happened before the caller received the handler back.
I propose a slight change in the API. We should add the listeners as an 
argument of the submit method, so we can set them on the Handler before the job 
itself is submitted. This way any status change should be signalled to the 
listener.


Diffs (updated)
-

  spark-client/src/main/java/org/apache/hive/spark/client/JobHandle.java 
44aa255a8271894ed3e787c3e7d1323628db63c4 
  spark-client/src/main/java/org/apache/hive/spark/client/JobHandleImpl.java 
17c8f40edd472682d5604f41980d06e60cc92893 
  spark-client/src/main/java/org/apache/hive/spark/client/SparkClient.java 
3e921a5d9b77966d368684ee7b6f1c861ac60e08 
  spark-client/src/main/java/org/apache/hive/spark/client/SparkClientImpl.java 
e2a30a76e0f7fe95d8a453f502311baa08abcbe2 
  spark-client/src/test/java/org/apache/hive/spark/client/TestJobHandle.java 
e8f352dce9f618573c2d79e9b8c59e19fad7298a 
  spark-client/src/test/java/org/apache/hive/spark/client/TestSparkClient.java 
b95cd7a05d44224b53bf2cef9170146b8b2eb4a8 

Diff: https://reviews.apache.org/r/53166/diff/


Testing
---

Unit tests modified and tested.
I also ran a simple query with HoS as the execution engine.


Thanks,

Barna Zsombor Klara

Re: Review Request 53166: HIVE-14910: Flaky test: TestSparkClient.testJobSubmission

[Peter Vary]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53166/#review153868
---



Hi Zsombor,

Thanks, nice, just a nit.

Still hoping someone with more Spark experience could review this code.

Thanks,
Peter


spark-client/src/test/java/org/apache/hive/spark/client/TestSparkClient.java 
(line 39)


nit: this one is not used


- Peter Vary


On Oct. 26, 2016, 8:47 a.m., Barna Zsombor Klara wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53166/
> ---
> 
> (Updated Oct. 26, 2016, 8:47 a.m.)
> 
> 
> Review request for hive, Mohit Sabharwal, Siddharth Seth, and Xuefu Zhang.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-14910: Flaky test: TestSparkClient.testJobSubmission
> I ran into this problem today while investigating a flaky test. I think the 
> failure is coming from this race condition: the listener can be added to the 
> JobHandle only after the job has been submitted. So there is no guarantee 
> that every method of the listener will be invoked, some state changes may 
> have happened before the caller received the handler back.
> I propose a slight change in the API. We should add the listeners as an 
> argument of the submit method, so we can set them on the Handler before the 
> job itself is submitted. This way any status change should be signalled to 
> the listener.
> 
> 
> Diffs
> -
> 
>   spark-client/src/main/java/org/apache/hive/spark/client/JobHandle.java 
> 44aa255a8271894ed3e787c3e7d1323628db63c4 
>   spark-client/src/main/java/org/apache/hive/spark/client/JobHandleImpl.java 
> 17c8f40edd472682d5604f41980d06e60cc92893 
>   spark-client/src/main/java/org/apache/hive/spark/client/SparkClient.java 
> 3e921a5d9b77966d368684ee7b6f1c861ac60e08 
>   
> spark-client/src/main/java/org/apache/hive/spark/client/SparkClientImpl.java 
> e2a30a76e0f7fe95d8a453f502311baa08abcbe2 
>   spark-client/src/test/java/org/apache/hive/spark/client/TestJobHandle.java 
> e8f352dce9f618573c2d79e9b8c59e19fad7298a 
>   
> spark-client/src/test/java/org/apache/hive/spark/client/TestSparkClient.java 
> b95cd7a05d44224b53bf2cef9170146b8b2eb4a8 
> 
> Diff: https://reviews.apache.org/r/53166/diff/
> 
> 
> Testing
> ---
> 
> Unit tests modified and tested.
> I also ran a simple query with HoS as the execution engine.
> 
> 
> Thanks,
> 
> Barna Zsombor Klara
> 
>

Re: Review Request 53166: HIVE-14910: Flaky test: TestSparkClient.testJobSubmission

[Barna Zsombor Klara]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53166/
---

(Updated Oct. 26, 2016, 8:47 a.m.)


Review request for hive, Mohit Sabharwal, Siddharth Seth, and Xuefu Zhang.


Changes
---

Initially I wanted to keep the changes to a minimum, but based on Peter's 
comments I decided to move the listeners into the constructor and make the list 
immutable.


Repository: hive-git


Description
---

HIVE-14910: Flaky test: TestSparkClient.testJobSubmission
I ran into this problem today while investigating a flaky test. I think the 
failure is coming from this race condition: the listener can be added to the 
JobHandle only after the job has been submitted. So there is no guarantee that 
every method of the listener will be invoked, some state changes may have 
happened before the caller received the handler back.
I propose a slight change in the API. We should add the listeners as an 
argument of the submit method, so we can set them on the Handler before the job 
itself is submitted. This way any status change should be signalled to the 
listener.


Diffs (updated)
-

  spark-client/src/main/java/org/apache/hive/spark/client/JobHandle.java 
44aa255a8271894ed3e787c3e7d1323628db63c4 
  spark-client/src/main/java/org/apache/hive/spark/client/JobHandleImpl.java 
17c8f40edd472682d5604f41980d06e60cc92893 
  spark-client/src/main/java/org/apache/hive/spark/client/SparkClient.java 
3e921a5d9b77966d368684ee7b6f1c861ac60e08 
  spark-client/src/main/java/org/apache/hive/spark/client/SparkClientImpl.java 
e2a30a76e0f7fe95d8a453f502311baa08abcbe2 
  spark-client/src/test/java/org/apache/hive/spark/client/TestJobHandle.java 
e8f352dce9f618573c2d79e9b8c59e19fad7298a 
  spark-client/src/test/java/org/apache/hive/spark/client/TestSparkClient.java 
b95cd7a05d44224b53bf2cef9170146b8b2eb4a8 

Diff: https://reviews.apache.org/r/53166/diff/


Testing
---

Unit tests modified and tested.
I also ran a simple query with HoS as the execution engine.


Thanks,

Barna Zsombor Klara

Re: Review Request 53166: HIVE-14910: Flaky test: TestSparkClient.testJobSubmission

[Peter Vary]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53166/#review153804
---



Thanks for the patch Zsombor!

Good to get rid of one more flaky test :)

I am not sure whether we want to keep the possibility to add new listeners 
after a job was started, or not.
- If we want to add more listeners after the job was started, then I think we 
should keep the JobHandleImpl.addListener method public, and leave it on the 
JobHandle interface too. If we chose this, at lease a comment should describe 
that after submission of the Job the listener calls are unpredictable because 
of the race conditions.
- If we do not want to add more listeners after the job was started, then we 
should add the listeners to the constructor of the JobHandleImpl, and keep the 
addListener method private

This is more nicety than anything else.

Otherwise the patch seems good to me. Thanks again!

I hope someone with more Spark experience will check it too.

Peter

- Peter Vary


On Oct. 25, 2016, 3:20 p.m., Barna Zsombor Klara wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53166/
> ---
> 
> (Updated Oct. 25, 2016, 3:20 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal, Siddharth Seth, and Xuefu Zhang.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-14910: Flaky test: TestSparkClient.testJobSubmission
> I ran into this problem today while investigating a flaky test. I think the 
> failure is coming from this race condition: the listener can be added to the 
> JobHandle only after the job has been submitted. So there is no guarantee 
> that every method of the listener will be invoked, some state changes may 
> have happened before the caller received the handler back.
> I propose a slight change in the API. We should add the listeners as an 
> argument of the submit method, so we can set them on the Handler before the 
> job itself is submitted. This way any status change should be signalled to 
> the listener.
> 
> 
> Diffs
> -
> 
>   spark-client/src/main/java/org/apache/hive/spark/client/JobHandle.java 
> 44aa255a8271894ed3e787c3e7d1323628db63c4 
>   spark-client/src/main/java/org/apache/hive/spark/client/JobHandleImpl.java 
> 17c8f40edd472682d5604f41980d06e60cc92893 
>   spark-client/src/main/java/org/apache/hive/spark/client/SparkClient.java 
> 3e921a5d9b77966d368684ee7b6f1c861ac60e08 
>   
> spark-client/src/main/java/org/apache/hive/spark/client/SparkClientImpl.java 
> e2a30a76e0f7fe95d8a453f502311baa08abcbe2 
>   
> spark-client/src/test/java/org/apache/hive/spark/client/TestSparkClient.java 
> b95cd7a05d44224b53bf2cef9170146b8b2eb4a8 
> 
> Diff: https://reviews.apache.org/r/53166/diff/
> 
> 
> Testing
> ---
> 
> Unit tests modified and tested.
> I also ran a simple query with HoS as the execution engine.
> 
> 
> Thanks,
> 
> Barna Zsombor Klara
> 
>

Review Request 53166: HIVE-14910: Flaky test: TestSparkClient.testJobSubmission

[Barna Zsombor Klara]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53166/
---

Review request for hive, Mohit Sabharwal, Siddharth Seth, and Xuefu Zhang.


Repository: hive-git


Description
---

HIVE-14910: Flaky test: TestSparkClient.testJobSubmission
I ran into this problem today while investigating a flaky test. I think the 
failure is coming from this race condition: the listener can be added to the 
JobHandle only after the job has been submitted. So there is no guarantee that 
every method of the listener will be invoked, some state changes may have 
happened before the caller received the handler back.
I propose a slight change in the API. We should add the listeners as an 
argument of the submit method, so we can set them on the Handler before the job 
itself is submitted. This way any status change should be signalled to the 
listener.


Diffs
-

  spark-client/src/main/java/org/apache/hive/spark/client/JobHandle.java 
44aa255a8271894ed3e787c3e7d1323628db63c4 
  spark-client/src/main/java/org/apache/hive/spark/client/JobHandleImpl.java 
17c8f40edd472682d5604f41980d06e60cc92893 
  spark-client/src/main/java/org/apache/hive/spark/client/SparkClient.java 
3e921a5d9b77966d368684ee7b6f1c861ac60e08 
  spark-client/src/main/java/org/apache/hive/spark/client/SparkClientImpl.java 
e2a30a76e0f7fe95d8a453f502311baa08abcbe2 
  spark-client/src/test/java/org/apache/hive/spark/client/TestSparkClient.java 
b95cd7a05d44224b53bf2cef9170146b8b2eb4a8 

Diff: https://reviews.apache.org/r/53166/diff/


Testing
---

Unit tests modified and tested.
I also ran a simple query with HoS as the execution engine.


Thanks,

Barna Zsombor Klara