LuciferYang commented on PR #57923:
URL: https://github.com/apache/spark/pull/57923#issuecomment-5357413646
All three applied. Where I went a different way from the suggestion:
1. `<message>` stays, and the PR body now carries the README note. It is a
well-trodden deviation rather than a new one: 27 other non-legacy conditions
carry a `<message>` placeholder, and `CANNOT_PARSE_TIMESTAMP`,
`INCOMPATIBLE_DATASOURCE_REGISTER`, `STREAMING_ASYNC_OPERATION_FAILED`,
`UNSUPPORTED_TABLE_CHANGE` and `SPILL_OUT_OF_MEMORY` all feed a builder's
exception message into it. `Option(e.getMessage).getOrElse(e.toString)` is
stricter than several of those, which pass a possibly-null `getMessage`
straight through.
2. SPARK-39647 is tightened, but through `checkError` rather than
`checkErrorMatchPVals`, and with a narrower pattern:
```scala
checkError(
exception = intercept[SparkException] {
makeBlockManager(8000, "timeoutExec")
},
condition = "UNABLE_TO_REGISTER_WITH_EXTERNAL_SHUFFLE_SERVICE",
sqlState = Some("58030"),
parameters = Map("message" ->
"(?s)java\\.util\\.concurrent\\.TimeoutException: .*"),
matchPVals = true)
```
The three-argument `checkErrorMatchPVals` passes `sqlState = None`, which
would have left this the only one of the three named conditions with no
SQLSTATE assertion. Changing 58030 would then go red in the other two and stay
green here. `checkError` takes `sqlState` and `matchPVals` together, and
`CSVSuite` already uses it that way.
The pattern is the FQCN plus its colon rather than `.*TimeoutException.*`,
because the latter passes on any text that merely mentions the word. That
prefix is deterministic, since `RuntimeException(Throwable)` sets
`detailMessage` to `cause.toString()`, and everything after it belongs to
Guava: the future's identity hash, and a `(plus N milliseconds, M nanoseconds
delay)` clause. I checked that both assertions bite by breaking them, 58030 to
58031 and the class name to `InterruptedException`, and each went red printing
the real value.
3. The hint is carried over with one word changed. The sibling says "not
sufficient space available to store the block", which reads as disk, and disk
cannot reach this throw. The call site fires when
`TempFileBasedBlockStoreUpdater.save()` returns `false`, and
`saveToDiskStore()` returns `Unit`, so a failed disk write throws an
`IOException` that `doPut` propagates instead of returning false. The only
non-throwing `false` left is a memory-only level whose `acquireStorageMemory`
fails. So it now reads:
> Failed to store block $blockId on $blockManagerId. This mostly happens
when there is not enough storage memory for the block and its storage level has
no disk fallback.
Since this text is the whole diagnostic value the error has, pointing it at
the one resource that cannot be the cause looked worse than shipping no hint at
all. `NettyBlockRpcServer.receive` has the same imprecision for the same reason
(its updater's `saveToDiskStore()` is `Unit` too). I left it alone to keep this
PR inside one subsystem, and the PR body records that this is a correction of
that phrasing rather than a divergence from it.
Agreed on the tautological `resolverClass` assertion, leaving it as is.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
