You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@auron.apache.org by "cxzl25 (via GitHub)" <gi...@apache.org> on 2026/01/22 17:41:44 UTC
[PR] Q14b [auron]
cxzl25 opened a new pull request, #1946:
URL: https://github.com/apache/auron/pull/1946
# Which issue does this PR close?
Closes #<issue_number>
# Rationale for this change
# What changes are included in this PR?
# Are there any user-facing changes?
# How was this patch tested?
--
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: commits-unsubscribe@auron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AURON #1937][CI] Enable TPCDS test q14b [auron]
Posted by "richox (via GitHub)" <gi...@apache.org>.
richox merged PR #1946:
URL: https://github.com/apache/auron/pull/1946
--
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: commits-unsubscribe@auron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AURON #1937][CI] Enable TPCDS test q14b [auron]
Posted by "yew1eb (via GitHub)" <gi...@apache.org>.
yew1eb commented on PR #1946:
URL: https://github.com/apache/auron/pull/1946#issuecomment-3787925938
Nice fix @cxzl25! Could you share the root cause and troubleshooting clues for this q14b issue?
--
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: commits-unsubscribe@auron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] q14b [auron]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1946:
URL: https://github.com/apache/auron/pull/1946#discussion_r2718055081
##########
spark-extension/src/main/scala/org/apache/spark/sql/auron/NativeConverters.scala:
##########
@@ -1405,9 +1406,20 @@ object NativeConverters extends Logging {
serialized: Array[Byte]): (E with Serializable, S) = {
Utils.tryWithResource(new ByteArrayInputStream(serialized)) { bis =>
Utils.tryWithResource(new ObjectInputStream(bis)) { ois =>
- val expr = ois.readObject().asInstanceOf[E with Serializable]
- val payload = ois.readObject().asInstanceOf[S with Serializable]
- (expr, payload)
+ def read(): (E with Serializable, S) = {
+ val expr = ois.readObject().asInstanceOf[E with Serializable]
+ val payload = ois.readObject().asInstanceOf[S with Serializable]
+ (expr, payload)
+ }
+ // Spark TaskMetrics#externalAccums is not thread-safe
+ val taskContext = TaskContext.get()
+ if (taskContext != null) {
+ taskContext.taskMetrics().synchronized {
Review Comment:
The synchronization on the entire taskMetrics() object may be overly broad and could impact performance. The comment on line 1414 mentions that "TaskMetrics#externalAccums is not thread-safe", but the synchronization is performed on the entire taskMetrics() object rather than on externalAccums specifically. This means the lock is held during the entire deserialization process (readObject calls), which could be time-consuming and might create unnecessary contention. Consider whether synchronization on a more specific object (like externalAccums itself) or using a finer-grained locking strategy would be more appropriate.
```suggestion
val metrics = taskContext.taskMetrics()
// Synchronize on the more specific externalAccums object when available,
// falling back to the whole metrics instance if reflection fails or is null.
val lockObj: AnyRef = try {
val fieldValue = FieldUtils.readField(metrics, "externalAccums", true)
fieldValue match {
case ref: AnyRef if ref != null => ref
case _ => metrics
}
} catch {
case _: Throwable => metrics
}
lockObj.synchronized {
```
--
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: commits-unsubscribe@auron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AURON #1937][CI] Enable TPCDS test q14b [auron]
Posted by "cxzl25 (via GitHub)" <gi...@apache.org>.
cxzl25 closed pull request #1946: [AURON #1937][CI] Enable TPCDS test q14b
URL: https://github.com/apache/auron/pull/1946
--
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: commits-unsubscribe@auron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] q14b [auron]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1946:
URL: https://github.com/apache/auron/pull/1946#discussion_r2717968256
##########
spark-extension/src/main/scala/org/apache/spark/sql/auron/NativeConverters.scala:
##########
@@ -1405,9 +1406,19 @@ object NativeConverters extends Logging {
serialized: Array[Byte]): (E with Serializable, S) = {
Utils.tryWithResource(new ByteArrayInputStream(serialized)) { bis =>
Utils.tryWithResource(new ObjectInputStream(bis)) { ois =>
- val expr = ois.readObject().asInstanceOf[E with Serializable]
- val payload = ois.readObject().asInstanceOf[S with Serializable]
- (expr, payload)
+ // Spark TaskMetrics#externalAccums is not thread-safe
+ val taskContext = TaskContext.get()
+ if (taskContext != null) {
+ taskContext.taskMetrics().synchronized {
+ val expr = ois.readObject().asInstanceOf[E with Serializable]
+ val payload = ois.readObject().asInstanceOf[S with Serializable]
+ (expr, payload)
+ }
+ } else {
+ val expr = ois.readObject().asInstanceOf[E with Serializable]
+ val payload = ois.readObject().asInstanceOf[S with Serializable]
+ (expr, payload)
Review Comment:
There is significant code duplication between the synchronized block (lines 1413-1415) and the else block (lines 1418-1420). The same two lines of code are repeated. Consider extracting the deserialization logic to avoid this duplication. For example, you could perform the readObject calls before the conditional and only wrap the specific operation that requires synchronization.
```suggestion
def read(): (E with Serializable, S) = {
val expr = ois.readObject().asInstanceOf[E with Serializable]
val payload = ois.readObject().asInstanceOf[S with Serializable]
(expr, payload)
}
// Spark TaskMetrics#externalAccums is not thread-safe
val taskContext = TaskContext.get()
if (taskContext != null) {
taskContext.taskMetrics().synchronized {
read()
}
} else {
read()
```
--
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: commits-unsubscribe@auron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] q14b [auron]
Posted by "cxzl25 (via GitHub)" <gi...@apache.org>.
cxzl25 closed pull request #1946: q14b
URL: https://github.com/apache/auron/pull/1946
--
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: commits-unsubscribe@auron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AURON #1937][CI] Enable TPCDS test q14b [auron]
Posted by "cxzl25 (via GitHub)" <gi...@apache.org>.
cxzl25 commented on PR #1946:
URL: https://github.com/apache/auron/pull/1946#issuecomment-3788513889
> Could you share the root cause and troubleshooting clues for this q14b issue
Judging from the call stack, the accumUpdates of TaskResult contains null values.
```java
ERROR TaskResultGetter: Exception while getting task result
java.lang.NullPointerException
at org.apache.spark.scheduler.TaskResultGetter$$anon$3.$anonfun$run$3(TaskResultGetter.scala:109)
```
The only place where `TaskMetrics#externalAccums` might be updated is `TaskMetrics#registerAccumulator`, which in turn is invoked via `taskContext.registerAccumulator(this)` called by the deserialization of `AccumulatorV2#readObject`. From the perspective of the code, it is highly unlikely for a null value to be written here.
Since `externalAccums` is an ArrayBuffer and not thread-safe, I tried adding the synchronized keyword to `TaskMetrics#registerAccumulator`, and the NPE issue was resolved. I then added some logging to record which threads were accessing this method concurrently, and ultimately identified that Auron's deserialization of expressions may be the root cause of this problem.
--
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: commits-unsubscribe@auron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org