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

Change strategy to test connectivity between ODBC driver and SQL plugin #69

Merged
merged 8 commits into from
May 20, 2021

Conversation

chloe-zh
Copy link
Contributor

@chloe-zh chloe-zh commented May 19, 2021

Description

Currently before establishes the connection between the ODBC driver and the OpenSearch server, the client tries to check the installed plugins in the current OpenSearch cluster by calling the _cat/plugins API. In this way, the connection will not be established if it does not detect the plugin with exactly the same name that was hard coded in the code base. It might cause compatibility issue once the SQL plugin is renamed, and it does not follow the common strategy to test the odbc connectivity as well.

Thus we changed the strategy in this PR to let it send a simple select 1 query to the server to check the sql plugin availability instead.

  • Case 1: SQL plugin is not installed
    Behavior: connectivity test fails
    image

  • Case 2: SQL plugin is installed but disabled
    Behavior: connectivity test fails
    image

  • Case 3: SQL plugin is installed and enabled, but somehow unhealthy (query execution failed)
    Behavior: connectivity test fails
    Similar error windows pop up with message: Connection error: [OpenSearch][SQL ODBC Driver][SQL Plugin] Connection error: SQL plugin is disabled, please enable the plugin to use this driver.

  • Case 4: SQL plugin is installed, enabled and in healthy status
    Behavior: connectivity test passes
    image

Issues Resolved

#57

Check List

  • All tests pass, including unit test, integration test and doctest
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@chloe-zh chloe-zh added the ODBC label May 19, 2021
@chloe-zh chloe-zh self-assigned this May 19, 2021
@chloe-zh chloe-zh changed the base branch from main to develop May 19, 2021 00:44
chloe-zh added 2 commits May 19, 2021 16:23
…-project#26

# Conflicts:
#	sql-odbc/src/sqlodbc/opensearch_communication.cpp
@chloe-zh chloe-zh marked this pull request as ready for review May 20, 2021 00:34
@chloe-zh chloe-zh requested review from dai-chen and penghuo May 20, 2021 00:53

if(!IsSQLPluginEnabled(error_details)) {
m_error_message +=
"The SQL plugin is disabled. The SQL plugin must be "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we add the doc link to guide customer how to enable SQL plugin.

}

if (response->HasClientError()) {
m_error_message += " Client error: '"
Copy link
Collaborator

@penghuo penghuo May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the action item if the customer receive "Client error" or "Response error". Could we reduce all the error into same pattern. e.g. "Error: detailReason"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the detailed error messages are only for log debug purpose, it will not be sent to the users. Yes we can merge them together.

Copy link
Collaborator

@penghuo penghuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change!

Copy link
Collaborator

@dai-chen dai-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes!

@chloe-zh chloe-zh merged commit ae0c482 into opensearch-project:develop May 20, 2021
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.

3 participants