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

Singleton service #2444

Merged
merged 8 commits into from Oct 13, 2016
Merged

Singleton service #2444

merged 8 commits into from Oct 13, 2016

Conversation

tardyp
Copy link
Member

@tardyp tardyp commented Oct 11, 2016

This class implements a generic Service that needs to be instantiated as a singleton according to its parameters.
It is a common use case to need this for accessing remote services.
Having a singleton to access a remote allows to limit the number of simultaneous access to the same services.
Thus, several completely independent buildbot services can use that singleton to access the service, and automatically synchronize themselves to not overwhelm it.

@mention-bot
Copy link

@tardyp, thanks for your PR! By analyzing the history of the files in this pull request, we identified @yetanotherion, @djmitche and @sa2ajj to be potential reviewers.

Copy link
Contributor

@sa2ajj sa2ajj left a comment

Choose a reason for hiding this comment

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

Looks quite useful

if name in parent.namedServices:
return defer.succeed(parent.namedServices[name])
try:
o = cls(*args, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

o? oh? oh, no!


@classmethod
def getName(cls, *args, **kwargs):
h = hashlib.sha1()
Copy link
Contributor

Choose a reason for hiding this comment

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

this PR seems to love one letter names

Copy link
Member Author

Choose a reason for hiding this comment

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

:) h sounds like hash which is a reserved word.
You are right

service.SingletonService.__init__(self)
self.name = self.getName(hyper_host, hyper_accesskey, hyper_secretkey)
# Prepare the parameters for the Docker Client object.
self.client_args = {'clouds': {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to have client_args public?

}}

def startService(self):
self.threadPool = threadpool.ThreadPool(
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

self.threadPool = threadpool.ThreadPool(
minthreads=1, maxthreads=self.MAX_THREADS, name='hyper')
self.threadPool.start()
self.client = Hyper(self.client_args)
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

This class implements a generic Service that needs to be instantiated as a singleton according to its parameters.
It is a common use case to need this for accessing remote services.
Having a singleton to access a remote allows to limit the number of simultaneous access to the same services.
Thus, several completely independent buildbot services can use that singleton to access the service, and automatically synchronize themselves to not overwhelm it.
Copy link
Contributor

Choose a reason for hiding this comment

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

Buildbot

Class method.
Takes same arguments as the constructor of the service.
Get a unique name for that instance of a service.
Used to decide if we need to create a new object.
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure I understand what this means.

Takes same arguments as the constructor of the service (plus the `parentService` at the beginning of the list).
Construct an instance of the service if needed, and place it at the beginning of the `parentService` service list.
Placing it at the beginning will guarantee that the singleton will be stopped after the other services.

Copy link
Contributor

Choose a reason for hiding this comment

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

one empty line here will be enough.

@@ -1,4 +1,3 @@
# This file is part of Buildbot. Buildbot is free software: you can
Copy link
Contributor

Choose a reason for hiding this comment

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

this line is probably removed by accident

o = cls(*args, **kwargs)
except Exception:
return defer.fail(failure.Failure())
o.name = name
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I like this forceful setting of .name

@@ -886,6 +886,44 @@ For example, a particular daily scheduler could be configured on multiple master
Therefore, in this method it is safe to reassign the "active" status to another instance.
This method may return a Deferred.

.. py:class:: SingletonService

This class implements a generic Service that needs to be instantiated as a singleton according to its parameters.
Copy link

Choose a reason for hiding this comment

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

if you can have n instances of this service but with different parameters this is not a singleton... for accessing to external services, we use normal services "mounted" into a known proxy (with an hardcoded name, which is not very elegant), for instance to be able to have 1 gerrit service talking to Gerrit 2.6 and another talking to Gerrit 2.11.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, this is a parametric singleton, which I argue is very similar concept.

Also, this idea here is that the end-user does not have to know there are singleton to be parametric.

The user will configure a HyperWorker with all the connection parameters in it, and then the HyperWorkers will automatically create the singleton as an implementation detail.

Copy link

@gsemet gsemet Oct 12, 2016

Choose a reason for hiding this comment

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

ok, but I advice to be very careful in defining it in the class doc for programmers. Of course, users do not care about this detail. But, for most programmers, Singleton

is a design pattern that restricts the instantiation of a class to one object

and

An implementation of the singleton pattern must:

  • ensure that only one instance of the singleton class ever exists; and
  • provide global access to that instance.

wikipedia

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you are right, we need to change the name
http://stackoverflow.com/questions/1050991/singleton-with-arguments-in-java

@codecov-io
Copy link

codecov-io commented Oct 12, 2016

Current coverage is 86.76% (diff: 100%)

Merging #2444 into master will increase coverage by 0.01%

@@             master      #2444   diff @@
==========================================
  Files           298        298          
  Lines         30940      30957    +17   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          26842      26860    +18   
+ Misses         4098       4097     -1   
  Partials          0          0          

Powered by Codecov. Last update f7be7bc...e3c6b05

This is not really a singleton, as this is designed to have multiple instance
at the same time.
@tardyp tardyp mentioned this pull request Oct 12, 2016
return defer.succeed(parent.namedServices[name])
try:
instance = cls(*args, **kwargs)
except Exception:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want catch all Exceptions here?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we transforms all exceptions into failures

There is one instance of this manager per host, accesskey, secretkey tuple.
This manager manages its own thread pull, as Hyper_sh is blocking
"""
name = "HyperLatentManager"
Copy link
Contributor

Choose a reason for hiding this comment

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

quotes

This manager manages its own thread pull, as Hyper_sh is blocking
"""
name = "HyperLatentManager"
MAX_THREADS = 5
Copy link
Contributor

Choose a reason for hiding this comment

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

do we use this value outside? if not _MAX_THREADS

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, this is eventually designed to be overriden

@tardyp
Copy link
Member Author

tardyp commented Oct 13, 2016

@sa2ajj I think I answered to all the reviews, I am going to merge this as soon as travis is done.

@tardyp tardyp merged commit 62dcf0c into buildbot:master Oct 13, 2016
@sa2ajj
Copy link
Contributor

sa2ajj commented Oct 13, 2016

@tardyp do you really want us to review the change?

@tardyp
Copy link
Member Author

tardyp commented Oct 13, 2016

@sa2ajj I do want reviews. I think I addressed everything, and I wanted to move forward, as I have several dependent branches.
If you have anymore issue, I'd be happy to address them in a follow up PR

@sa2ajj
Copy link
Contributor

sa2ajj commented Oct 14, 2016

You substantially changed things since the original PR, if I had been the author, I'd wait for the other review, not just Travis results.

@tardyp
Copy link
Member Author

tardyp commented Oct 14, 2016

@sa2ajj I just added two trivial comments since your last review, and a note in relnotes
5ad6326
e3c6b05

There are 800loc to be reviewed in #2446 which are harder to review if the dependent branch is not merged.

I am really sorry if I upset you, this was really not my intention.

I'm just trying to use the sparse review time the best as I can so that we can move as fast as we can.

I have a lot of time now to develop buildbot, and I won't in a few weeks/months, when I'll need to transition to my new paid job (you know how it is).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants