-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
SQL: handle SQL not being available in a more graceful way #43665
Conversation
Adds a new qa sub-project that explicitly disables SQL XPack module in Gradle.
Pinging @elastic/es-search |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good, left a question.
public void testJdbcExceptionMessage() throws SQLException { | ||
try (Connection c = esJdbc()) { | ||
SQLException e = expectThrows(SQLException.class, () -> c.prepareStatement("SELECT * FROM bla").executeQuery()); | ||
assertTrue(e.getMessage().startsWith("X-Pack/SQL do not seem to be available on the Elasticsearch node using the access path")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do -> does
+ "] contains unrecognized parameter: [mode]"; | ||
private static final String SQL_NOT_AVAILABLE_ERROR_MESSAGE_HTTPMETHOD = "Incorrect HTTP method for uri [" + SQL_QUERY_REST_ENDPOINT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this case tested somewhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The special case of sql not being installed/disabled had no tests before this one.
@elastic/es-core-infra is there any other solution that you might think of for this integration test to be possible? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Add test for SQL not being available error message in JDBC. * Add a new qa sub-project that explicitly disables SQL XPack module in Gradle. (cherry picked from commit 8a1ac8a)
* Add test for SQL not being available error message in JDBC. * Add a new qa sub-project that explicitly disables SQL XPack module in Gradle. (cherry picked from commit 8a1ac8a)
* Add test for SQL not being available error message in JDBC. * Add a new qa sub-project that explicitly disables SQL XPack module in Gradle. (cherry picked from commit 8a1ac8a)
When #36149 was merged, the default requests the JDBC driver was making changed slightly (the
mode
parameter was not a parameter anymore) and, also, the rest requests the ES server was receiving were handled slightly differently in the RestController.So differently, that the error message being generated when a SQL request was made to a cluster that didn't have SQL installed (or disabled) was different.
This PR changes this error message handling and, also, adds a much needed integration test to handles this scenario. The test is a separate QA project which, in Gradle configuration, has SQL disabled.
Fixes #41279.