Trino Gravitino connector throws NullPointerException #11365 - #11426
Conversation
… to provide more context in error messages. Updated catch blocks to include the request URI aswell for better debugging.
diqiu50
left a comment
diqiu50
left a comment
There was a problem hiding this comment.
I think we should focus on finding the root cause of the issue, instead of just handling the NPE.
Code Coverage Report
Files
|
|
@diqiu50 Alright, no problem. I will take a deeper dive into it. I had previously assumed it was just unhandled edge cases as listed in the description, but I will spend more time reproducing and testing the given method over the weekend, and I will get back to you about my progress on Monday Afternoon. |
…nd the error to lie in the buildURI method of the HTTPClient class. The method is not properly handling the case where the server URI is invalid, which leads to a NullPointerException when it tries to build the request URI. I have added a check for null and throw an IllegalArgumentException with a more descriptive message when the server URI is invalid. This should help in identifying the issue more clearly in the future.
|
@diqiu50 I spent some time revisiting the bug report and came to the conclusion it was due to an overly simple buildUri function inside of HTTPClient.java. I came to this conclusion by first acknowledging the fact that the issue happens after the CatalogConnectorManager starts, not before. So, in order to jump to DefaultOAuth2TokenProvider, which, after testing, worked fine, I went to GravitinoAuthProvider, which was alright as well. Then I moved to the scheduled path, and followed until I found a path for possible error: Within HTTPClient.execute() I found room for error. I saw that the argument request, the parameter within the call, was built from buildUri() within the file. I then scrolled to the buildUri() function and found it was only manually joining the parameters uri and path using the String.format() function, then simply returning it. So it was an unchecked URI that was then passed to HC5, which could return an NPE. My fix was to first validate the URI by making a separate URI variable requestUri, and building it. If its required properties were still null, scheme or host, then I would throw a RESTException stating such, otherwise I'd return it as expected. I didn't change the manually joining parameters to preserve already existing tests and code flow, while still addressing the issue. I also added one more unit test to ensure an invalid URI is handled before the execution, as was the issue I observed. Importantly, I haven't had the time to create my own Kubernetes environment to perfectly replicate the bug, as I'm newer to this repository, but I feel that I was correctly following the bug's trail, and I'm pretty confident in my solution. It'd be great if someone with the particular environment could test my proposal. Thank you |
- Use buildUri() result directly as requestUri instead of calling request.getUri() afterwards, which was both roundabout and declared an unreachable URISyntaxException catch block - Remove the NullPointerException catch that matched on HC5's internal string literal "Endpoint"; buildUri() already validates scheme and host are non-null, so HC5 will always have a valid authority set on the request and cannot reach that code path
|
@Octavi00 Thanks for your contribution. Your fix looks good. I removed some dead code. |
…terException #11365 (#11426) (#11562) **Cherry-pick Information:** - Original commit: 732adb1 - Target branch: `branch-1.3` - Status: ✅ Clean cherry-pick (no conflicts) Co-authored-by: Octavio Herrera Contreras <156601957+Octavi00@users.noreply.github.com> Co-authored-by: yuhui <hui@datastrato.com>
2 participants
What changes were proposed in this pull request?
Added specific exception handling to avoid previous bland nullptr exceptions.
Why are the changes needed?
Fix: #11365
Does this PR introduce any user-facing change?
No
How was this patch tested?
Added two unit tests to prove the specific exceptions' behavior.
IMPORTANT: since the bug fix was considered urgent, and I have no previous Kubernetes environment, I chose to utilize unit tests to replicate common exceptions that would allow me to create specific cases for such.