Skip to content
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

[SPARK-25921][Follow Up][PySpark] Fix barrier task run without BarrierTaskContext while python worker reuse #23435

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
4 participants
@xuanyuanking
Copy link
Contributor

commented Jan 3, 2019

What changes were proposed in this pull request?

It's the follow-up PR for #22962, contains the following works:

  • Remove __init__ in TaskContext and BarrierTaskContext.
  • Add more comments to explain the fix.
  • Rewrite UT in a new class.

How was this patch tested?

New UT in test_taskcontext.py

@xuanyuanking

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2019

cc @HyukjinKwon and @cloud-fan.
Sorry for the late about this follow-up, please have a look.

@SparkQA

This comment has been minimized.

Copy link

commented Jan 3, 2019

Test build #100674 has finished for PR 23435 at commit 0cf822f.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
@HyukjinKwon
Copy link
Member

left a comment

Looks fine if the tests pass

Show resolved Hide resolved python/pyspark/tests/test_taskcontext.py Outdated
Show resolved Hide resolved python/pyspark/tests/test_taskcontext.py Outdated
Show resolved Hide resolved python/pyspark/tests/test_taskcontext.py Outdated
@xuanyuanking

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2019

@HyukjinKwon The newly added UT can pass in python2.7 and pypy, but fail in pyhton3. It seems that the worker reuse didn't take effect in python3, I'm looking into this, not sure it's a bug or not.

@SparkQA

This comment has been minimized.

Copy link

commented Jan 3, 2019

Test build #100676 has finished for PR 23435 at commit a5c20db.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
@xuanyuanking

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2019

It seems that the worker reuse didn't take effect in python3, I'm looking into this, not sure it's a bug or not.

It's a bug that worker reuse loses efficacy caused by the unexpected return of checking the end of stream logic in python worker, I'll give another PR and JIRA tomorrow to fix it, this PR will continue after the problem fix.

@HyukjinKwon

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

Let's fix #23470 first.

asfgit pushed a commit that referenced this pull request Jan 9, 2019

[SPARK-26549][PYSPARK] Fix for python worker reuse take no effect for…
… parallelize lazy iterable range

## What changes were proposed in this pull request?

During the follow-up work(#23435) for PySpark worker reuse scenario, we found that the worker reuse takes no effect for `sc.parallelize(xrange(...))`. It happened because of the specialize rdd.parallelize logic for xrange(introduced in #3264) generated data by lazy iterable range, which don't need to use the passed-in iterator. But this will break the end of stream checking in python worker and finally cause worker reuse takes no effect. See more details in [SPARK-26549](https://issues.apache.org/jira/browse/SPARK-26549) description.

We fix this by force using the passed-in iterator.

## How was this patch tested?
New UT in test_worker.py.

Closes #23470 from xuanyuanking/SPARK-26549.

Authored-by: Yuanjian Li <xyliyuanjian@gmail.com>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
@SparkQA

This comment has been minimized.

Copy link

commented Jan 10, 2019

Test build #101012 has finished for PR 23435 at commit eedd445.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
@HyukjinKwon

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

Merged to master.

@HyukjinKwon

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

This followup was not merged into branch-2.4 although the main PR went into branch-2.4 due to conflicts. Since it's rather stylic changes, I think it's okay not to backport this followup.

To reduce the diff between master and branch-2.4, we can backport it too if anyone thinks it should.

@asfgit asfgit closed this in 98e831d Jan 11, 2019

@xuanyuanking

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2019

 I think it's okay not to backport this followup.

Yea, agree.
Thanks Hyukjin and Felix for your review.

@xuanyuanking xuanyuanking deleted the xuanyuanking:SPARK-25921-follow branch Jan 11, 2019

stczwd added a commit to stczwd/spark that referenced this pull request Feb 18, 2019

[SPARK-26549][PYSPARK] Fix for python worker reuse take no effect for…
… parallelize lazy iterable range

## What changes were proposed in this pull request?

During the follow-up work(apache#23435) for PySpark worker reuse scenario, we found that the worker reuse takes no effect for `sc.parallelize(xrange(...))`. It happened because of the specialize rdd.parallelize logic for xrange(introduced in apache#3264) generated data by lazy iterable range, which don't need to use the passed-in iterator. But this will break the end of stream checking in python worker and finally cause worker reuse takes no effect. See more details in [SPARK-26549](https://issues.apache.org/jira/browse/SPARK-26549) description.

We fix this by force using the passed-in iterator.

## How was this patch tested?
New UT in test_worker.py.

Closes apache#23470 from xuanyuanking/SPARK-26549.

Authored-by: Yuanjian Li <xyliyuanjian@gmail.com>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

stczwd added a commit to stczwd/spark that referenced this pull request Feb 18, 2019

[SPARK-25921][FOLLOW UP][PYSPARK] Fix barrier task run without Barrie…
…rTaskContext while python worker reuse

## What changes were proposed in this pull request?

It's the follow-up PR for apache#22962, contains the following works:
- Remove `__init__` in TaskContext and BarrierTaskContext.
- Add more comments to explain the fix.
- Rewrite UT in a new class.

## How was this patch tested?

New UT in test_taskcontext.py

Closes apache#23435 from xuanyuanking/SPARK-25921-follow.

Authored-by: Yuanjian Li <xyliyuanjian@gmail.com>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

wangyum added a commit to wangyum/spark that referenced this pull request Mar 14, 2019

[SPARK-25921][FOLLOW UP][PYSPARK] Fix barrier task run without Barrie…
…rTaskContext while python worker reuse

## What changes were proposed in this pull request?

It's the follow-up PR for apache#22962, contains the following works:
- Remove `__init__` in TaskContext and BarrierTaskContext.
- Add more comments to explain the fix.
- Rewrite UT in a new class.

## How was this patch tested?

New UT in test_taskcontext.py

Closes apache#23435 from xuanyuanking/SPARK-25921-follow.

Authored-by: Yuanjian Li <xyliyuanjian@gmail.com>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.