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

Provide QueryStatus V2 API #1371

Closed
wants to merge 3 commits into from
Closed

Conversation

arouel
Copy link
Contributor

@arouel arouel commented Apr 28, 2023

Overview

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes The errorCode and errorMessage fields of QueryStatus enum are not thread-safe #1370

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am modifying authorization mechanisms
    • I am adding new credentials
    • I am modifying OCSP code
    • I am adding a new dependency
  3. Please describe how your code solves the related issue.

    • Introduce a new thread-safe data structure called QueryStatusV2 to keep the behavior of the QueryStatus enum for backwards compatibility.
      • The new data structure provides more metadata about the query status to better understand what is going on.
    • Introduce a new query status method at SnowflakeResultSet called getQueryStatus which return a QueryStatusV2.
      • The feature is only implemented for SFAsyncResultSet, and SnowflakeResultSetV1 throws a SnowflakeLoggedFeatureNotSupportedException.
    • Introduce a new query status method at SFSession called getQueryStatusV2 which return a QueryStatusV2.
    • Keep the existing methods and behavior stable for backwards compatibility.

Pre-review checklist

  • This change has passed precommit
  • I have reviewed code coverage report for my PR in (Sonarqube)

@arouel arouel requested a review from a team as a code owner April 28, 2023 10:46
@sfc-gh-igarish
Copy link
Collaborator

sfc-gh-igarish commented Apr 28, 2023

If original issue is related to thread safety then why can't we fix it in existing API? Why we need a new V2 API?

@arouel
Copy link
Contributor Author

arouel commented May 2, 2023

@sfc-gh-igarish the existing data structure is an enum, and the only option I see to make it thread-safe would be to use thread-local variables for errorCode and errorMessage, but this may not solve the problem in all the use cases I can think of. On the other hand, we need more metadata about the query itself, therefore we opted for a new API call. What do you suggest moving forward?

@sfc-gh-igarish
Copy link
Collaborator

sfc-gh-igarish commented Aug 3, 2023

Any public API signature changes is a breaking change as it used by existing users. Please think of making existing API thread safe.

@sfc-gh-hchaturvedi
Copy link
Collaborator

Another suggestion would be to add the new API without changing the existing API so that they both can co-exist without breaking backwards compatibility. Also @arouel you mentioned there are other reasons why this API cannot be thread safe, care to elaborate?

@sfc-gh-igarish
Copy link
Collaborator

@arouel Please check PR comments.

@@ -153,7 +153,7 @@ public void addQueryToActiveQueryList(String queryID) {
* @return enum of type QueryStatus indicating the query's status
* @throws SQLException
*/
public QueryStatus getQueryStatus(String queryID) throws SQLException {
public JsonNode getQueryMetadata(String queryID) throws SQLException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this as public?

* @return enum of type QueryStatus indicating the query's status
* @throws SQLException
*/
public QueryStatus getQueryStatus(String queryID) throws SQLException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely worth some deprecation comment now.

* @return an instance containing query metadata
* @throws SQLException
*/
QueryStatusV2 getQueryStatus() throws SQLException;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we should name it consistently to existing function - getStatusV2?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also why new function is not taking queryId as an argument?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because this is a function on resultSet - resultSet knows its queryId (even provides it by getQueryId).

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is more about async execution. Are we providing resultset object before completed the query?

@@ -153,7 +153,7 @@ public void addQueryToActiveQueryList(String queryID) {
* @return enum of type QueryStatus indicating the query's status
Copy link
Contributor

Choose a reason for hiding this comment

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

This information is not accurate anymore?

@sfc-gh-pfus
Copy link
Contributor

Implemented in #1579

@sfc-gh-pfus sfc-gh-pfus closed this Dec 8, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The errorCode and errorMessage fields of QueryStatus enum are not thread-safe
4 participants