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

Add a prefix to worker thread #1358

Merged
merged 4 commits into from Aug 23, 2019

Conversation

@Eldinnie
Copy link
Member

Eldinnie commented Feb 28, 2019

This adds a prefix of Bot:<id>:worker: to the name of the worker threads.
Fixes #1332

@Eldinnie Eldinnie requested a review from jsmnbom Feb 28, 2019
@jsmnbom

This comment has been minimized.

Copy link
Member

jsmnbom commented Feb 28, 2019

Looks pretty good all around, got some comments though.

  1. This doesn't 100% fix #1332 does it? It only adds the prefix to worker threads, is there anything preventing us from doing the same with the dispatcher and updater thread?
  2. Should it maybe be called ptb:<id>:worker: or telegram:<id>:worker or something like that? Just to make it 100% explicit what the thread is?
  3. I'm not really a fan of having a method just for testing purposes. Can't you just do dp._Dispatcher__async_threads in the test?
@Eldinnie

This comment has been minimized.

Copy link
Member Author

Eldinnie commented Feb 28, 2019

@jsmnbom I will address your notes later, want to see if this indeed fixes Travis' behavior

@bmxp

This comment has been minimized.

Copy link

bmxp commented Mar 19, 2019

I am looking forward for your improvements and will try out asap ...

@jsmnbom

This comment has been minimized.

Copy link
Member

jsmnbom commented Apr 4, 2019

@Eldinnie any updates on this? Do you agree with my comments? I can implement them if you dont' have time ^ ^

@NickKush

This comment has been minimized.

Copy link

NickKush commented May 6, 2019

Any updates about this PR? I have the same issue as #1332 ( ._.)

@bmxp

This comment has been minimized.

Copy link

bmxp commented May 7, 2019

As long as this PR is not accepted into Release 12 I have tricked around with something like this which is not beautiful but does the necessary change:

pretty_thread_names = True
prefix_programname = "YourProgramnameHere"
prefix_worker_thread = "Worker"

# theUpdater contains the telegram Updater Object

if pretty_thread_names:
    try:
        for t in theUpdater._Updater__threads:
          if 'dispatcher' in t.name: 
            t.name = prefix_programname+' Dispatcher'
            
          if 'updater' in t.name: 
            t.name = prefix_programname+' Updater'
            
        for t in theUpdater.dispatcher._Dispatcher__async_threads:
          *_, num = t.name.split('_')
          t.name = '{0} {1} {2}'.format(prefix_programname, prefix_worker_thread, num) if num.isnumeric() else num

    except:
        pass
@Eldinnie Eldinnie changed the base branch from master to V12 Jun 6, 2019
Eldinnie added 2 commits Feb 28, 2019
This adds a prefix of `Bot:<id>:worker:` to the name of the worker threads.
Fixes #1332
@Eldinnie Eldinnie force-pushed the fix_1332 branch from 6648fc3 to dab532b Jun 6, 2019
@Eldinnie

This comment has been minimized.

Copy link
Member Author

Eldinnie commented Jun 6, 2019

Renamed all threads and changed base.

@Eldinnie

This comment has been minimized.

Copy link
Member Author

Eldinnie commented Jun 6, 2019

@jsmnbom care to take another look?

@Eldinnie Eldinnie self-assigned this Jun 6, 2019
@jsmnbom

This comment has been minimized.

Copy link
Member

jsmnbom commented Jun 6, 2019

This mostly looks good now, but i still don't think i can see my comment 3 adressed?

  1. I'm not really a fan of having a method just for testing purposes. Can't you just do dp._Dispatcher__async_threads in the test?
@Eldinnie

This comment has been minimized.

Copy link
Member Author

Eldinnie commented Jun 6, 2019

No you can't.
It's a protected variable

@jsmnbom
jsmnbom approved these changes Jun 6, 2019
Copy link
Member

jsmnbom left a comment

Alright, if that's not possible then, this looks good :)

@Eldinnie Eldinnie merged commit edad6e8 into V12 Aug 23, 2019
1 of 3 checks passed
1 of 3 checks passed
Codacy/PR Quality Review Codacy was unable to analyse your pull request.
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Eldinnie Eldinnie deleted the fix_1332 branch Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.