Skip to content

[SPARK-43125][CONNECT] Fix Connect Server Can't Handle Exception With Null Message#40780

Closed
Hisoka-X wants to merge 1 commit intoapache:masterfrom
Hisoka-X:SPARK-43125_Exception_NPE
Closed

[SPARK-43125][CONNECT] Fix Connect Server Can't Handle Exception With Null Message#40780
Hisoka-X wants to merge 1 commit intoapache:masterfrom
Hisoka-X:SPARK-43125_Exception_NPE

Conversation

@Hisoka-X
Copy link
Member

What changes were proposed in this pull request?

Fix the bug when Connect Server throw Exception without message.

Why are the changes needed?

Fix bug

Does this PR introduce any user-facing change?

No

How was this patch tested?

Unnecessary

@Hisoka-X
Copy link
Member Author

@martin-g @HyukjinKwon PTAL. Thanks

@LuciferYang
Copy link
Contributor

Could you add a test case for this scenario?

@LuciferYang
Copy link
Contributor

hmm.. there is no [SERVER] tag, we should remove if from pr title

@Hisoka-X Hisoka-X changed the title [SPARK-43125][CONNECT][SERVER] Fix Connect Server Can't Handle Exception With Null Message [SPARK-43125][CONNECT] Fix Connect Server Can't Handle Exception With Null Message Apr 14, 2023
@HyukjinKwon
Copy link
Member

Merged to master and branch-3.4.

HyukjinKwon pushed a commit that referenced this pull request Apr 14, 2023
… Null Message

### What changes were proposed in this pull request?
Fix the bug when Connect Server throw Exception without message.

### Why are the changes needed?
Fix bug

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Unnecessary

Closes #40780 from Hisoka-X/SPARK-43125_Exception_NPE.

Authored-by: Hisoka <fanjiaeminem@qq.com>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
(cherry picked from commit ea49637)
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
@Hisoka-X Hisoka-X deleted the SPARK-43125_Exception_NPE branch April 14, 2023 03:48
@WweiL
Copy link
Contributor

WweiL commented Apr 14, 2023

@Hisoka-X @HyukjinKwon Any reason we drop the abbreviate?

@Hisoka-X
Copy link
Member Author

@Hisoka-X @HyukjinKwon Any reason we drop the abbreviate?

Sorry, I don't get it. What do you mean about drop the abbreviate? This PR just do null check.

@WweiL
Copy link
Contributor

WweiL commented Apr 17, 2023

@Hisoka-X @HyukjinKwon Any reason we drop the abbreviate?

Sorry, I don't get it. What do you mean about drop the abbreviate? This PR just do null check.

Ah sorry! I didn't see you did the abbreviate in the top, please ignore this!

1 similar comment
@WweiL
Copy link
Contributor

WweiL commented Apr 17, 2023

@Hisoka-X @HyukjinKwon Any reason we drop the abbreviate?

Sorry, I don't get it. What do you mean about drop the abbreviate? This PR just do null check.

Ah sorry! I didn't see you did the abbreviate in the top, please ignore this!

@WweiL
Copy link
Contributor

WweiL commented Apr 17, 2023

@Hisoka-X
Copy link
Member Author

@Hisoka-X I think we also change this line? https://github.com/apache/spark/pull/40780/files#diff-d21a96890e660df398527183a191b0d0c73521aada856e2491c33243f269fdceR128

I could add it to my ongoing PR #40785

OK for me, but I just want to remind, add null check just want to avoid throw NPE in setMessage, not want to change null to ""

@WweiL
Copy link
Contributor

WweiL commented Apr 18, 2023

@Hisoka-X I think we also change this line? https://github.com/apache/spark/pull/40780/files#diff-d21a96890e660df398527183a191b0d0c73521aada856e2491c33243f269fdceR128
I could add it to my ongoing PR #40785

OK for me, but I just want to remind, add null check just want to avoid throw NPE in setMessage, not want to change null to ""

I see. With additional look I think here it's better to keep what it is. as Status's description is nullable:

private Status(Code code, @Nullable String description, @Nullable Throwable cause) {
    this.code = checkNotNull(code, "code");
    this.description = description;
    this.cause = cause;
  }

public Status withDescription(String description) {
    if (Objects.equal(this.description, description)) {
      return this;
    }
    return new Status(this.code, description, this.cause);
  }

snmvaughan pushed a commit to snmvaughan/spark that referenced this pull request Jun 20, 2023
… Null Message

### What changes were proposed in this pull request?
Fix the bug when Connect Server throw Exception without message.

### Why are the changes needed?
Fix bug

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Unnecessary

Closes apache#40780 from Hisoka-X/SPARK-43125_Exception_NPE.

Authored-by: Hisoka <fanjiaeminem@qq.com>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
(cherry picked from commit ea49637)
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
GladwinLee pushed a commit to lyft/spark that referenced this pull request Oct 10, 2023
… Null Message

### What changes were proposed in this pull request?
Fix the bug when Connect Server throw Exception without message.

### Why are the changes needed?
Fix bug

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Unnecessary

Closes apache#40780 from Hisoka-X/SPARK-43125_Exception_NPE.

Authored-by: Hisoka <fanjiaeminem@qq.com>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
(cherry picked from commit ea49637)
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
catalinii pushed a commit to lyft/spark that referenced this pull request Oct 10, 2023
… Null Message

### What changes were proposed in this pull request?
Fix the bug when Connect Server throw Exception without message.

### Why are the changes needed?
Fix bug

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Unnecessary

Closes apache#40780 from Hisoka-X/SPARK-43125_Exception_NPE.

Authored-by: Hisoka <fanjiaeminem@qq.com>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
(cherry picked from commit ea49637)
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

Comments