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

SNOW-965966: Remove unnecessary TODOs from DefaultResultStreamProvider #1607

Conversation

sfc-gh-ext-simba-jf
Copy link
Collaborator

@sfc-gh-ext-simba-jf sfc-gh-ext-simba-jf commented Jan 16, 2024

Overview

SNOW-965966 Move http client creation to HttpUtil class from DefaultResultStreamProvider

External contributors - please answer these questions before submitting a pull request. Thanks!

Please answer these questions before submitting your pull requests. Thanks!

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

    Fixes

  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.

    Moved the http client creation out of DefaultResultStreamProvider

Pre-review checklist

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

@sfc-gh-ext-simba-jf sfc-gh-ext-simba-jf marked this pull request as ready for review January 16, 2024 22:24
@sfc-gh-ext-simba-jf sfc-gh-ext-simba-jf requested a review from a team as a code owner January 16, 2024 22:24
Copy link

SonarQube Quality Gate

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Contributor

@sfc-gh-pfus sfc-gh-pfus left a comment

Choose a reason for hiding this comment

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

Hi @sfc-gh-ext-simba-jf ! I have a couple of questions here:

  1. What is the main change here? By looking at the code I can see that this is only a bit of refactoring, we use the same HttpClient retrieved from HttpUtil etc. I'm certainly missing the root change.
  2. How did you test it? I see no change in tests, so maybe you can describe in the PR description what manual tests you performed?

@sfc-gh-ext-simba-jf
Copy link
Collaborator Author

Hi @sfc-gh-pfus, the request for this ticket was basically just to refactor the code to utilize the connection pool manager initialized by the HttpUtil class. For testing I used the hang_webserver as well as the existing IT and unit tests.

@sfc-gh-dprzybysz sfc-gh-dprzybysz changed the title Move http client creation to HttpUtil from DefaultResultStreamProvider SNOW-965966: Remove unnecessary TODOs from DefaultResultStreamProvider Feb 9, 2024
@sfc-gh-dprzybysz sfc-gh-dprzybysz merged commit dcefd7b into master Feb 9, 2024
35 of 36 checks passed
@sfc-gh-dprzybysz sfc-gh-dprzybysz deleted the SNOW-965966-Use-HttpUtil-when-creating-http-clients-for-DefaultResultStreamProvider branch February 9, 2024 08:22
@github-actions github-actions bot locked and limited conversation to collaborators Feb 9, 2024
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.

4 participants