Re: [PR] [SPARK-48928] Log Warning for Calling .unpersist() on Locally Checkpointed RDDs [spark]
mridulm commented on PR #47391: URL: https://github.com/apache/spark/pull/47391#issuecomment-2238279977 It is unclear to me what the value of adding this to unpersist is. Use of local checkpoint'ing, irrespective of whether it was persist'ed or not, is risky. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48928] Log Warning for Calling .unpersist() on Locally Checkpointed RDDs [spark]
Ngone51 commented on PR #47391: URL: https://github.com/apache/spark/pull/47391#issuecomment-2238627870 > It is unclear to me what the value of adding this to unpersist is. I think it at least gives users an explict warning in first place rather than relying on the lazily triggered error msg in the case of the misuse of local checkpoint. And local checkpoint is still used by the public API today, `Dataset#localCheckpoint()`. So might be ok to add it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48928] Log Warning for Calling .unpersist() on Locally Checkpointed RDDs [spark]
mridulm commented on PR #47391: URL: https://github.com/apache/spark/pull/47391#issuecomment-2241285255 My point is, use of persist/unpersist is orthogonal to this - so why special case only here ? Better would be to add it when user invokes local checkpoint -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48928] Log Warning for Calling .unpersist() on Locally Checkpointed RDDs [spark]
mingkangli-db commented on PR #47391: URL: https://github.com/apache/spark/pull/47391#issuecomment-2244052554 > My point is, use of persist/unpersist is orthogonal to this - so why special case only here ? Yes, you are right that the use of persist/unpersist can be orthogonal to this. However, we special case because calling`.unpersist()` _after local checkpointing_ will lead to job failure if any recomputation happens. `.unpersist()` is usually a safe operation because the RDD can still be recomputed if `unpersist()` is called, but not in this case. > Better would be to add it when user invokes local checkpoint When user invokes `.localCheckpoint()`, this is fine as long as they understand the risk of having no fault-tolerance, but the combo `localCheckpoint()` + `unpersist()` can happen when there is no executor failure, so this is just to make users aware of the potential consequences. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48928] Log Warning for Calling .unpersist() on Locally Checkpointed RDDs [spark]
mridulm commented on PR #47391: URL: https://github.com/apache/spark/pull/47391#issuecomment-2244505394 If there is no executor failure, whether unpersist is called or not, there should be no failure as there is locally checkpointed data on the node. Are you seeing this not being the case ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48928] Log Warning for Calling .unpersist() on Locally Checkpointed RDDs [spark]
mingkangli-db commented on PR #47391:
URL: https://github.com/apache/spark/pull/47391#issuecomment-2245804911
> If there is no executor failure/loss, whether unpersist is called or not,
there should be no failure as there is locally checkpointed data on the node.
Are you seeing this not being the case ? (It has been quite a while since I
looked at this though, so hopefully I am not misremembering)
You're right that when there is no executor failure, there is no failure
when you call `.localCheckpoint()` and `.unpersist()` _by themselves_. However,
if you perform any action on the RDD _afterwards_, it will lead to an
exception. This happens because, after unpersisting, the lineage is cut, and
the RDD cannot be recomputed. For example:
```
val checkpointedDf = sql("select * from range(10)")
.localCheckpoint(eager = false)
val rdd =
checkpointedDf.queryExecution.analyzed.asInstanceOf[LogicalRDD].rdd
checkpointedDf.collect()
rdd.unpersist()
// EXCEPTION HAPPENS HERE
checkpointedDf.collect()
```
The last line results in an exception because the checkpointed RDD is
evicted, even without node failure. Logging a warning after `rdd.unpersist()`
would make it clearer to users what the consequence would be.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48928] Log Warning for Calling .unpersist() on Locally Checkpointed RDDs [spark]
mridulm commented on PR #47391: URL: https://github.com/apache/spark/pull/47391#issuecomment-2246142777 Ah, I see what you mean - sorry, I had forgotten that this is simply relying on storage level and not actually writing to the local disk (similar to hdfs for regular checkpoint). Make sense to add this. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48928] Log Warning for Calling .unpersist() on Locally Checkpointed RDDs [spark]
mridulm closed pull request #47391: [SPARK-48928] Log Warning for Calling .unpersist() on Locally Checkpointed RDDs URL: https://github.com/apache/spark/pull/47391 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48928] Log Warning for Calling .unpersist() on Locally Checkpointed RDDs [spark]
mridulm commented on PR #47391: URL: https://github.com/apache/spark/pull/47391#issuecomment-2246146791 Merged to master, thanks for fixing this @mingkangli-db ! Thanks for the reviews @jiangxb1987 , @Ngone51 :-) Also thanks to everyone for answering my queries - I really was misremembering how local checkpoint was happening. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
