changgyoopark-db commented on code in PR #49370:
URL: https://github.com/apache/spark/pull/49370#discussion_r1913233357
##########
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:
The order of read operations is 1. executionCompleted (load-acquire) -> 2. {
sizeLimitReached || deadlineReached }.
After 1., there are two possible states.
1. Either completed or an error is recorded in the observer: gotResponse
must be "true" so the entire block won't be executed. In other words, if
something's been consumed, the new code block in this change won't be executed.
2. None recorded: since !sizeLimitReached && !gotResponse, there is no way
further data will be produced by any executors for the query, therefore raising
an error will be the correct action.
But, thinking about it a little bit further, there can be an unfortunate
occasion where deadlineReached becomes true just before entering the while loop
(line 237), e.g., when the OS scheduler kicks in.
-> Hm...
##########
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:
The order of read operations is 1. executionCompleted (load-acquire) -> 2. {
sizeLimitReached || deadlineReached }.
After 1., there are two possible states.
1. Either completed or an error is recorded in the observer: gotResponse
must be "true" so the entire block won't be executed. In other words, if
something's been consumed, the new code block in this change won't be executed.
2. None recorded: since !sizeLimitReached && !gotResponse, there is no way
further data will be produced by any executors for the query, therefore raising
an error will be the correct action.
But, thinking about it a little bit further, there can be an unfortunate
occasion where deadlineReached becomes true just before entering the while loop
(line 237), e.g., when the OS scheduler kicks in.
-> Hm...
--
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
