Skip to content

Comments

3 http client#8

Merged
Moasib-Arif merged 12 commits intomainfrom
3-HTTP-Client
Jan 17, 2024
Merged

3 http client#8
Moasib-Arif merged 12 commits intomainfrom
3-HTTP-Client

Conversation

@Moasib-Arif
Copy link
Contributor

Created a HTTP Client that can send get() and post() requests using the python httpclient library.

Changes:

  • Implemented get, post and request methods in the http
  • Implemented exponential backoff (it will retry when request fails)
  • Added test coverage for methods, backoff and proprogate kwargs

Copy link
Contributor

@mikeAdamss mikeAdamss left a comment

Choose a reason for hiding this comment

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

the timout aspects of this are getting nasty, bad steer from me, hard to change the timeout when its in the decorator not the class.

for now - just use backoff_max from the instantiation for max_time in the decorator and lose the kwarg approach. That should greatly simplify it. It someone really needs to send two requests with different timeouts from the same app they'll just have to reinstantiate the client (its not a super likely scenario).

I can see why you've gone with urllib here (factor things out) btw but I'd be tempted to see how it looks with requests, its very batteries included and simple. If its more verbose but the code is simpler that's ok.

Last thing, be sure to try the client a bit in anger 😄 , I think the kwarg propogation is off a bit at the moment and it;'d be really obvious if you play around with this a bit, i.e

from dpytools.http_clients.http_custom import HttpClient

client = HttpClient()

client.get("") # <--- try some actual urls, some that work, some that dont. check you're getting those retry logs etc.

@mikeAdamss mikeAdamss linked an issue Jan 14, 2024 that may be closed by this pull request
3 tasks
@Moasib-Arif Moasib-Arif marked this pull request as ready for review January 15, 2024 09:34
Copy link
Contributor

@mikeAdamss mikeAdamss left a comment

Choose a reason for hiding this comment

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

minor tidyup changes only

Copy link
Contributor

@mikeAdamss mikeAdamss left a comment

Choose a reason for hiding this comment

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

LGTM

@Moasib-Arif Moasib-Arif merged commit 00c29ea into main Jan 17, 2024
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.

implement backoff enabled http client

2 participants