Skip to content

fix(bq_driver): Set native gRPC DNS resolver for HTAPI reads#1547

Merged
sachinpro merged 9 commits into
mainfrom
dns_issueee
Jun 8, 2026
Merged

fix(bq_driver): Set native gRPC DNS resolver for HTAPI reads#1547
sachinpro merged 9 commits into
mainfrom
dns_issueee

Conversation

@Anshu6250

Copy link
Copy Markdown
Collaborator

No description provided.

@Anshu6250 Anshu6250 changed the title test fix: Set native gRPC DNS resolver for HTAPI reads Jun 1, 2026
@Anshu6250 Anshu6250 changed the title fix: Set native gRPC DNS resolver for HTAPI reads fix(bq_driver): Set native gRPC DNS resolver for HTAPI reads Jun 1, 2026
@Anshu6250 Anshu6250 changed the title fix(bq_driver): Set native gRPC DNS resolver for HTAPI reads fix(bq_driver) : Set native gRPC DNS resolver for HTAPI reads Jun 1, 2026
@Anshu6250 Anshu6250 changed the title fix(bq_driver) : Set native gRPC DNS resolver for HTAPI reads fix(bq_driver): Set native gRPC DNS resolver for HTAPI reads Jun 1, 2026
_putenv_s("GRPC_DNS_RESOLVER", "native");
#else
setenv("GRPC_DNS_RESOLVER", "native", 1);
#endif

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You will need to clear the cached stream:

    stmt_handle.ClearReadRowsStream();
    stmt_handle.ClearReadRowsIterator();

for the subsequent calls of ReadNextResultsFromStream to work.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I missed that you made changes in google/cloud/odbc/bq_driver/internal/odbc_sql_execute_utils.cc.

The fix has to be done during SQLExecute and no change duiring SQLFetch is needed.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Have verified but setting env variable on the fly like this, don't switch the GRPC channel , it still uses the one from which it was created by the library, We can either close the older channel and recreate the client which can have issue with multithreading or we can set the DNS resolver to native windows one at the start itself in all cases. @sachinpro . We can take a call.

@sachinpro sachinpro Jun 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Lets set the DNS resolver to native windows in all cases. Thanks for verifying this.

@sachinpro sachinpro marked this pull request as ready for review June 8, 2026 14:22
@sachinpro sachinpro requested a review from a team as a code owner June 8, 2026 14:22
@sachinpro

Copy link
Copy Markdown
Collaborator

@shivamd-gpartner @Anshu6250 I have simplified the logic here. Can you please manually verify if it works?

@Anshu6250

Anshu6250 commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator Author

@shivamd-gpartner @Anshu6250 I have simplified the logic here. Can you please manually verify if it works?
hi @sachinpro ,
i have already tested this logic , this works but it introduces a noticeable increase in table loading time in Power BI. I am currently investigating the source of the performance regression.

@sachinpro

Copy link
Copy Markdown
Collaborator

I am merging this since it is a safe change. The GHA checks were successful.

@sachinpro sachinpro merged commit 476562f into main Jun 8, 2026
25 of 28 checks passed
@sachinpro sachinpro deleted the dns_issueee branch June 8, 2026 14:58
@sachinpro

Copy link
Copy Markdown
Collaborator

@shivamd-gpartner @Anshu6250 I have simplified the logic here. Can you please manually verify if it works?
hi @sachinpro ,
i have already tested this logic , this works but it introduces a noticeable increase in table loading time in Power BI. I am currently investigating the source of the performance regression.

Sorry, I missed this reply. Are you sure GRPC_DNS_RESOLVER=native is the cause? It should only improve performance.

You might've tested with your previous logic. Can you test with the latest changes on main?

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