You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by LiangchangZ <gi...@git.apache.org> on 2018/05/28 15:44:20 UTC
[GitHub] spark pull request #21445: [SPARK-24404][SS] Increase currentEpoch when meet...
GitHub user LiangchangZ opened a pull request:
https://github.com/apache/spark/pull/21445
[SPARK-24404][SS] Increase currentEpoch when meet a EpochMarker in Co…
## What changes were proposed in this pull request?
Invoke EpochTracker.incrementCurrentEpoch() before send ReportPartitionOffset message to epochCoordinator. Detail see the jira SPARK-24404.
## How was this patch tested?
Run a CP application based on pull request #21353 #21332 and #21293
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/LiangchangZ/spark cp-branch
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/21445.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 #21445
----
commit 01304f2bb582cd827b459e2ec5d17f06804e25c3
Author: zhuliangchang <zh...@...>
Date: 2018-05-28T15:40:08Z
[SPARK-24404][SS] Increase currentEpoch when meet a EpochMarker in ContinuousQueuedDataReader.next() in CP mode
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21445: [SPARK-24404][SS] Increase currentEpoch when meet a Epoc...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/spark/pull/21445
@LiangchangZ
Looks like the patch is needed only with #21353 #21332 #21293 as of now, right? If then please state the condition in JIRA issue description as well as PR's description so that we don't get confused.
There's a case that reader and writer are composed together in a task (current state of continuous processing), and then after this patch it will be two places which increase epoch for the same thread. Please note that I'm commenting on top of current implementation, not considering #21353 #21332 #21293.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21445: [SPARK-24404][SS] Increase currentEpoch when meet a Epoc...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21445
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21445: [SPARK-24404][SS] Increase currentEpoch when meet a Epoc...
Posted by advancedxy <gi...@git.apache.org>.
Github user advancedxy commented on the issue:
https://github.com/apache/spark/pull/21445
> I think the best way to do it is to make the shuffle writer responsible for incrementing the epoch within its task, the same way the data source writer does currently.
Yeah, @LiangchangZ please consider this way. The writer part of a task is responsible to pull data from upstream. It's more consistent and wouldn't break existing logic.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21445: [SPARK-24404][SS] Increase currentEpoch when meet a Epoc...
Posted by jose-torres <gi...@git.apache.org>.
Github user jose-torres commented on the issue:
https://github.com/apache/spark/pull/21445
Agreed with the above. Have you run the unit tests? I wouldn't expect them to pass with this change.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21445: [SPARK-24404][SS] Increase currentEpoch when meet a Epoc...
Posted by xuanyuanking <gi...@git.apache.org>.
Github user xuanyuanking commented on the issue:
https://github.com/apache/spark/pull/21445
```
Looks like the patch is needed only with #21353 #21332 #21293 as of now, right?
```
@HeartSaVioR Yes, sorry for the late explanation. The background is we are running POC based on #21353 #21332 #21293 and the latest master, including the work of queue rdd reader/writer by @jose-torres. Greatly thanks for the work of #21239, we can complete all status operation after fix this bug. So we think we should report this to let you know.
```
Please note that I'm commenting on top of current implementation, not considering #21353 #21332 #21293.
```
Got it, owing to some pressure within internal requirement for CP, we running over these 3 patches, but we'll follow closely with all your works and hope to contribute into CP.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21445: [SPARK-24404][SS] Increase currentEpoch when meet a Epoc...
Posted by LiangchangZ <gi...@git.apache.org>.
Github user LiangchangZ commented on the issue:
https://github.com/apache/spark/pull/21445
> Have you run the unit tests? I wouldn't expect them to pass with this change.
@jose-torres UT test of ContinuousAggregationSuite, ContinuousSuite failed.
But I think EpochTracker need to support those cases no matter the reader and writer are composed in a task or in different tasks in the future. In the real CP situation, reader and writer may be always in different tasks, right?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21445: [SPARK-24404][SS] Increase currentEpoch when meet a Epoc...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21445
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21445: [SPARK-24404][SS] Increase currentEpoch when meet a Epoc...
Posted by LiangchangZ <gi...@git.apache.org>.
Github user LiangchangZ commented on the issue:
https://github.com/apache/spark/pull/21445
@jose-torres @HeartSaVioR @advancedxy Got it，thanks for reply.
> I think the best way to do it is to make the shuffle writer responsible for incrementing the epoch within its task, the same way the data source writer does currently
I will try this way. thanks again
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21445: [SPARK-24404][SS] Increase currentEpoch when meet a Epoc...
Posted by LiangchangZ <gi...@git.apache.org>.
Github user LiangchangZ commented on the issue:
https://github.com/apache/spark/pull/21445
cc @jose-torres @xuanyuanking
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21445: [SPARK-24404][SS] Increase currentEpoch when meet a Epoc...
Posted by LiangchangZ <gi...@git.apache.org>.
Github user LiangchangZ commented on the issue:
https://github.com/apache/spark/pull/21445
> Looks like the patch is needed only with #21353 #21332 #21293 as of now, right? If then please
> state the condition in JIRA issue description as well as PR's description so that we don't get confused
@HeartSaVioR yes, I have updated JIRA issue description as well as PR's description, sorry for the confusion.
> Please note that I'm commenting on top of current implementation, not considering #21353 #21332 #21293
Got it, thanks for reply
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21445: [SPARK-24404][SS] Increase currentEpoch when meet a Epoc...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21445
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21445: [SPARK-24404][SS] Increase currentEpoch when meet a Epoc...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21445
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21445: [SPARK-24404][SS] Increase currentEpoch when meet a Epoc...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/spark/pull/21445
@LiangchangZ
> In the real CP situation, reader and writer may be always in different tasks, right?
Continuous mode already supports some valid use cases, and putting all in one task would be fastest in such use cases though tasks can be parallelized by partition. Unless we have valid reason to separate reader and writer even in non-shuffle query, it would be better to keep it as it is.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21445: [SPARK-24404][SS] Increase currentEpoch when meet a Epoc...
Posted by jose-torres <gi...@git.apache.org>.
Github user jose-torres commented on the issue:
https://github.com/apache/spark/pull/21445
Sure, we need to support this, but the approach in this PR doesn't work if it breaks existing tests.
I think the best way to do it is to make the shuffle writer responsible for incrementing the epoch within its task, the same way the data source writer does currently.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org