Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RD-11521: report connection failures to the user #469

Conversation

bgaidioz
Copy link
Collaborator

@bgaidioz bgaidioz commented Jul 26, 2024

Before the pool was global, the code logic was that a connection failure to FDW would be an internal bug, FDW should always be available, and the connection pool should be provisioned to match the expected usage.

To review carefully if this behavior is what we want, and is implemented properly: To report connection failures to the user, we should isolate the connection failure SQLException (those are identified with code 08xxx), and process these ones as errors that are normal to hit sometimes:

  • for execute, validate and getProgramDescription, they are caught are the message is reported (as a String or ErrorMessage with positions when needed). The validate and getProgramDescription need positions, I've put positions spanning the full code.
  • for hover, we probably should ignore it and and do not report type info,
  • for connection errors occuring in metadata caches, we do not report the error to the user (I think), and since we can't connect, store an empty list of matches. That entry will be wiped after the usual timeout.

The PR only adds tests to execute,validate and getProgramDescription and LSP calls.

@miguelbranco80 miguelbranco80 changed the title RD:11521: report connection failures to the user RD-11521: report connection failures to the user Jul 30, 2024
@miguelbranco80
Copy link
Contributor

One comment is that we found a slightly different behaviour when doing LSP calls vs validate/execute calls. So we should indeed be sure for the behaviour we want for LSP. Probably it is indeed safe to ignore as you mention for the hover.

@bgaidioz bgaidioz force-pushed the RD-11521-execution-crash-report-raw-client-api-compiler-service-exception branch from 1d09d9b to 5923c2f Compare July 31, 2024 11:23
@bgaidioz
Copy link
Collaborator Author

bgaidioz commented Jul 31, 2024

One comment is that we found a slightly different behaviour when doing LSP calls vs validate/execute calls. So we should indeed be sure for the behaviour we want for LSP. Probably it is indeed safe to ignore as you mention for the hover.

I added tests that trigger both errors for LSP calls, ensuring they return nothing silently. There was even a place missing the try.

// hover returns nothing
val hoverResponse = runHover(compilerService, joe, "SELECT * FROM example.airports", Pos(1, 17))
assert(hoverResponse.completion.isEmpty)
val hoverResponse2 =
  runHover(compilerService, joe, "SELECT * FROM example.airports WHERE :id = airport_id", Pos(1, 40))
assert(hoverResponse2.completion.isEmpty)
// we get no word completions
val wordCompletionResponse = runWordCompletion(compilerService, joe, "SELECT * FROM exa", "exa", Pos(1, 17))
assert(wordCompletionResponse.completions.isEmpty)
// we get no dot completions
val dotCompletionResponse = runDotCompletion(compilerService, joe, "SELECT * FROM example.", Pos(1, 22))
assert(dotCompletionResponse.completions.isEmpty)                     

@bgaidioz bgaidioz force-pushed the RD-11521-execution-crash-report-raw-client-api-compiler-service-exception branch from 5923c2f to 823815a Compare August 1, 2024 09:37
@bgaidioz bgaidioz requested a review from miguelbranco80 August 1, 2024 15:44
@bgaidioz bgaidioz force-pushed the RD-11521-execution-crash-report-raw-client-api-compiler-service-exception branch from 05eaaa6 to 6bd93ec Compare August 2, 2024 10:29
@bgaidioz bgaidioz merged commit 3bb31a3 into main Aug 2, 2024
6 checks passed
@bgaidioz bgaidioz deleted the RD-11521-execution-crash-report-raw-client-api-compiler-service-exception branch August 2, 2024 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants