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

Revert "[SPARK-49808][SQL] Fix a deadlock in subquery execution due o lazy vals" #48362

Closed
wants to merge 1 commit into from

Conversation

zhengruifeng
Copy link
Contributor

@zhengruifeng zhengruifeng commented Oct 7, 2024

This reverts commit d7abddc.

We had offline discussion, and @JoshRosen pointed out that:

The use of LazyTry vs. a non-try Lazy wrappers needs discussion:
LazyTry caches failures. As a result, there is a potential risk of cancellation exceptions being cached here.
This is possibly an issue in case AQE interrupts subquery execution threads during planning, or otherwise can interrupt a subset of a query plan.

@mridulm
Copy link
Contributor

mridulm commented Oct 7, 2024

Can we add some details on why we are reverting to the description please ? For example: bug still exists in scenario foo, or new bug introduced for xyz, etc ?
Thanks

@zhengruifeng
Copy link
Contributor Author

zhengruifeng commented Oct 7, 2024

@mridulm thanks, let me update the description:
the reason is that the fix potentially causes other issues, so still need discussion and investigation.

@HyukjinKwon
Copy link
Member

I am fine with this. If there's a concern to be discussed, let's open a PR again, and discuss it there.

@HyukjinKwon
Copy link
Member

Merged to master.

@zhengruifeng zhengruifeng deleted the revert_fix_deadlock branch October 7, 2024 09:46
@tedyu
Copy link
Contributor

tedyu commented Oct 8, 2024

What if we introduce the following wrapper in place of the Scala lazy value ?

class LazyRetry[T](compute: => T) {
  private var initialized = false
  private var value: Option[T] = None

  def get: T = synchronized {
    if (!initialized) {
      try {
        value = Some(compute)
        initialized = true
      } catch {
        case e: Exception =>
          // So that it can retry on the next call
          initialized = false
          throw e
      }
    }
    value.getOrElse(throw new RuntimeException("Computation not successful"))
  }
}

This wrapper doesn't cache exceptions.

himadripal pushed a commit to himadripal/spark that referenced this pull request Oct 19, 2024
… lazy vals"

This reverts commit d7abddc.

We had offline discussion, and JoshRosen pointed out that:
> The use of LazyTry vs. a non-try Lazy wrappers needs discussion:
LazyTry caches failures. As a result, there is a potential risk of cancellation exceptions being cached here.
This is possibly an issue in case AQE interrupts subquery execution threads during planning, or otherwise can interrupt a subset of a query plan.

Closes apache#48362 from zhengruifeng/revert_fix_deadlock.

Authored-by: Ruifeng Zheng <ruifengz@apache.org>
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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants