[SPARK-20872][SQL] ShuffleExchange.nodeName should handle null coordinator #18095
Conversation
There was a problem hiding this comment.
should we adjust this pattern match condition at other places in the class below as well?
There was a problem hiding this comment.
I had left the other match conditions unchanged so that this change itself can be small and contained, because I'd need to verify whether or not the other match conditions should only be invoked with some valid Option[ExchangeCoordinator] value.
Anyway, I checked the two other coordinator match { ... } cases below, and both of them should still work fine to change to match on _ instead of None. But semantically both of these cases should only be invoked on the Driver side, where the coordinator shouldn't be null. I'm okay to change these two cases if reviewers suggest so.
There was a problem hiding this comment.
Let's do it then, that will enable future readers easier to understand the code.
|
LGTM, and I think it would be great if you can also address Sameer's comment. |
|
Also cc @gatorsmile |
rednaxelafx
force-pushed
the
shuffleexchange-nodename
branch
from
23aff2a to
a65ac4f
Compare
|
Updated patch to address the comments on making two other match conditions on |
|
Test build #77307 has finished for PR 18095 at commit
|
|
|
||
| // NOTE: coordinator can be null after serialization/deserialization, | ||
| // e.g. it can be null on the Executor side | ||
|
|
There was a problem hiding this comment.
nit: remove this empty line.
There was a problem hiding this comment.
This note is meant to be a class internal comment, and not a comment on override lazy val metrics = Map(, so I'd say leaving the added empty line here makes more sense. Would you agree?
|
Thanks, LGTM too |
|
Test build #77309 has finished for PR 18095 at commit
|
…nator ## What changes were proposed in this pull request? A one-liner change in `ShuffleExchange.nodeName` to cover the case when `coordinator` is `null`, so that the match expression is exhaustive. Please refer to [SPARK-20872](https://issues.apache.org/jira/browse/SPARK-20872) for a description of the symptoms. TL;DR is that inspecting a `ShuffleExchange` (directly or transitively) on the Executor side can hit a case where the `coordinator` field of a `ShuffleExchange` is null, and thus will trigger a `MatchError` in `ShuffleExchange.nodeName()`'s inexhaustive match expression. Also changed two other match conditions in `ShuffleExchange` on the `coordinator` field to be consistent. ## How was this patch tested? Manually tested this change with a case where the `coordinator` is null to make sure `ShuffleExchange.nodeName` doesn't throw a `MatchError` any more. Author: Kris Mok <kris.mok@databricks.com> Closes #18095 from rednaxelafx/shuffleexchange-nodename. (cherry picked from commit c0b3e45) Signed-off-by: Xiao Li <gatorsmile@gmail.com>
|
Thanks! Merging to master/2.2 |
5 participants
What changes were proposed in this pull request?
A one-liner change in
ShuffleExchange.nodeNameto cover the case whencoordinatorisnull, so that the match expression is exhaustive.Please refer to SPARK-20872 for a description of the symptoms.
TL;DR is that inspecting a
ShuffleExchange(directly or transitively) on the Executor side can hit a case where thecoordinatorfield of aShuffleExchangeis null, and thus will trigger aMatchErrorinShuffleExchange.nodeName()'s inexhaustive match expression.Also changed two other match conditions in
ShuffleExchangeon thecoordinatorfield to be consistent.How was this patch tested?
Manually tested this change with a case where the
coordinatoris null to make sureShuffleExchange.nodeNamedoesn't throw aMatchErrorany more.