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

Telegram Connection pool is full (and other tweaks) #5307

Merged
merged 3 commits into from
Sep 9, 2016
Merged

Telegram Connection pool is full (and other tweaks) #5307

merged 3 commits into from
Sep 9, 2016

Conversation

Gobberwart
Copy link
Contributor

Short Description:

Increase connection pool size to eliminate connection pool is full errors.
Removed unnecessary code from get_player_stats
Minor text tweaks (replace "spin up" with "start")

Fixes/Resolves/Closes (please use correct syntax):

- Increase connection pool (remove pool is full errors)
- Remove json read for player_stats
- Replaced "spin up" with "start"
@mention-bot
Copy link

@Gobberwart, thanks for your PR! By analyzing the annotation information on this pull request, we identified @DBa2016, @askovpen and @solderzzc to be potential reviewers

@solderzzc
Copy link
Contributor

Yes, the 'Connection pool is full' message is the only thing I can see on the console.

@Gobberwart
Copy link
Contributor Author

Hmmm CI failed with error

======================================================================
FAIL: test_api_call_throttle_should_fail (tests.api_wrapper_test.TestApiWrapper)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/virtualenv/python2.7.9/lib/python2.7/site-packages/timeout_decorator/timeout_decorator.py", line 69, in new_function
    return function(*args, **kwargs)
  File "/home/travis/build/PokemonGoF/PokemonGo-Bot/tests/api_wrapper_test.py", line 116, in test_api_call_throttle_should_fail
    request.call()
AssertionError: TimeoutError not raised

You think that's related? Shouldn't be but not sure... Any thoughts?

@Gobberwart
Copy link
Contributor Author

    @timeout(1) # expects a timeout
    def test_api_call_throttle_should_fail(self):
        request = FakeApi().create_request()
        request.is_response_valid = MagicMock(return_value=True)
        request.requests_per_seconds = 5

        with self.assertRaises(TimeoutError):
            for i in range(request.requests_pe

@Gobberwart Gobberwart closed this Sep 8, 2016
@Gobberwart Gobberwart reopened this Sep 8, 2016
@Gobberwart
Copy link
Contributor Author

Never mind, build passed now without me changing anything. Should be ready to merge if someone wants to approve it.

class TelegramClass:

update_id = None

session = None
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing at all. Was part of something I was testing, now unneccessary. Removing before merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in latest commit. Good catch, thanks.

@Gobberwart
Copy link
Contributor Author

@sohje Lots of comments on things that are not relevant to this PR. Yes, Telegram_handler.py needs a cleanup but not here.

@sohje
Copy link
Contributor

sohje commented Sep 9, 2016

@Gobberwart You already done some cleanup with this PR. That's why i commented. This PR about fix pool size, half cleanup also added?

request.CON_POOL_SIZE = 16

@solderzzc
Copy link
Contributor

solderzzc commented Sep 9, 2016

👍

Approved with PullApprove

@solderzzc
Copy link
Contributor

Let's remove the mass log with no much meaning on console.

@Gobberwart
Copy link
Contributor Author

Gobberwart commented Sep 9, 2016

@sohje I only updated things that irritated me. In fact, this PR was originally going to be just about removing code from get_player_stats but I figured I'd look at connection pool while I was at it and that's probably more important.

So yeah, not really much of a cleanup going on. I'll look into the other stuff later.

@sohje
Copy link
Contributor

sohje commented Sep 9, 2016

@Gobberwart Okay, good. I dont mean any harm 👍

@Gobberwart
Copy link
Contributor Author

@sohje All good mate, just don't want the PR bogged down in unrelated minutiae.

@Gobberwart
Copy link
Contributor Author

OK to merge now then? Please approve.

@solderzzc
Copy link
Contributor

solderzzc commented Sep 9, 2016

👍 Don't pretend you can not merge :)

Approved with PullApprove

@Gobberwart
Copy link
Contributor Author

LOL I know I can, I just prefer to let someone else check my work first :)

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.

5 participants