juliuszsompolski commented on code in PR #49370:
URL: https://github.com/apache/spark/pull/49370#discussion_r1912949576
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/execution/ExecuteGrpcResponseSender.scala:
##########
@@ -319,7 +325,14 @@ private[connect] class ExecuteGrpcResponseSender[T <:
Message](
log"waitingForResults=${MDC(WAIT_RESULT_TIME, consumeSleep /
NANOS_PER_MILLIS.toDouble)} ms " +
log"waitingForSend=${MDC(WAIT_SEND_TIME, sendSleep /
NANOS_PER_MILLIS.toDouble)} ms")
// scalastyle:on line.size.limit
- grpcObserver.onCompleted()
+ if (executionCompleted && !sizeLimitReached) {
Review Comment:
Why not just
```
else if (executionCompleted) {
```
?
If it is because executionCompleted or sizeLimitReached can change
concurrently from the few lines of `} else if (sizeLimitReached ||
deadlineReached) {` before?
If so, is the non-atomic check `executionCompleted && !sizeLimitReached`
safe?
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/execution/ExecuteThreadRunner.scala:
##########
@@ -63,6 +63,16 @@ private[connect] class ExecuteThreadRunner(executeHolder:
ExecuteHolder) extends
}
}
+ /**
+ * Checks if the execution is completed.
+ *
+ * @return
+ * true if the execution is completed.
+ */
+ private[connect] def isCompleted(): Boolean = {
+ state.getAcquire() == ThreadState.completed
+ }
Review Comment:
This still depends on our code reaching and executing the state transition
into completed... How does it help for "ExecuteThreadRunner fails to record the
completion/error to the observer (e.g., due to OOM)" where I understand the
thread would just die / not reach the transition?
Edit: I think I got it, it's about errors on the RPC sender i.e. handler
side, not execution thread side.
--
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
