workers using *state.State

2015-09-08 Thread William Reade 
People keep writing them, in defiance of sane layering and explicit
instructions, for the most embarrassingly trivial tasks
(statushistorypruner? dblogpruner? txnpruner? *all* of those can and should
pass through a simple api facade, not just dance off to play with the
direct-db-access fairies.)

There is no justification for *any* of those things to see a *state.State,
and I'm going to start treating new workers that violate layering this way
as deliberate sabotage attempts. Leads who have overseen the introduction
of those workers, sort it out.
-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: workers using *state.State

2015-09-08 Thread Menno Smits 
Hey Will,

FWIW, I'm responsible for 2 of them - dblogpruner and txnpruner. They were
created before I'd ever heard anything about workers not using *state.State
directly, certainly before the recent push to clean such workers up.
They're not all that new and weren't created in violation of explicit
instructions (the instructions came later).

When I created these workers I looked at the existing code and saw that
workers which only ran within the state servers used State directly so
that's what I did. These workers are very much state server specific so it
seemed sensible to take this approach.

I will update these workers to go via the API and cards already exist on
our team's board. It's just a matter of finding time amongst everything
else.

- Menno







On 8 September 2015 at 19:12, William Reade 
wrote:

> People keep writing them, in defiance of sane layering and explicit
> instructions, for the most embarrassingly trivial tasks
> (statushistorypruner? dblogpruner? txnpruner? *all* of those can and should
> pass through a simple api facade, not just dance off to play with the
> direct-db-access fairies.)
>
> There is no justification for *any* of those things to see a *state.State,
> and I'm going to start treating new workers that violate layering this way
> as deliberate sabotage attempts. Leads who have overseen the introduction
> of those workers, sort it out.
>
> --
> Juju-dev mailing list
> Juju-dev@lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>
>
-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: workers using *state.State

2015-09-08 Thread Ian Booth 
Those workers below aren't the only ones. There's also minunits and peergrouper
workers.

No-one does these things on purpose. Just last week I caught and rejected a pull
request to introduce a new worker depending on state directly. People make
mistakes. Perhaps we should introduce a test which fails if state is imported
into any worker code. We have similar tests already for other forbidden imports.

On 08/09/15 17:12, William Reade wrote:
> People keep writing them, in defiance of sane layering and explicit
> instructions, for the most embarrassingly trivial tasks
> (statushistorypruner? dblogpruner? txnpruner? *all* of those can and should
> pass through a simple api facade, not just dance off to play with the
> direct-db-access fairies.)
> 
> There is no justification for *any* of those things to see a *state.State,
> and I'm going to start treating new workers that violate layering this way
> as deliberate sabotage attempts. Leads who have overseen the introduction
> of those workers, sort it out.
> 
> 
> 

-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: workers using *state.State

2015-09-08 Thread Andrew Wilkins 
On Wed, Sep 9, 2015 at 6:14 AM Ian Booth  wrote:

> Those workers below aren't the only ones. There's also minunits and
> peergrouper
> workers.
>
> No-one does these things on purpose. Just last week I caught and rejected
> a pull
> request to introduce a new worker depending on state directly. People make
> mistakes. Perhaps we should introduce a test which fails if state is
> imported
> into any worker code. We have similar tests already for other forbidden
> imports.
>

+1. I was thinking the same thing, and eventually that test should be
increased to other packages too.

On 08/09/15 17:12, William Reade wrote:
> > People keep writing them, in defiance of sane layering and explicit
> > instructions, for the most embarrassingly trivial tasks
> > (statushistorypruner? dblogpruner? txnpruner? *all* of those can and
> should
> > pass through a simple api facade, not just dance off to play with the
> > direct-db-access fairies.)
> >
> > There is no justification for *any* of those things to see a
> *state.State,
> > and I'm going to start treating new workers that violate layering this
> way
> > as deliberate sabotage attempts. Leads who have overseen the introduction
> > of those workers, sort it out.
> >
> >
> >
>
> --
> Juju-dev mailing list
> Juju-dev@lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>
-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: workers using *state.State

2015-09-08 Thread Tim Penhey 
On 09/09/15 11:22, Andrew Wilkins wrote:
> On Wed, Sep 9, 2015 at 6:14 AM Ian Booth  <mailto:ian.bo...@canonical.com>> wrote:
> 
> Those workers below aren't the only ones. There's also minunits and
> peergrouper
> workers.
> 
> No-one does these things on purpose. Just last week I caught and
> rejected a pull
> request to introduce a new worker depending on state directly.
> People make
> mistakes. Perhaps we should introduce a test which fails if state is
> imported
> into any worker code. We have similar tests already for other
> forbidden imports.
> 
> 
> +1. I was thinking the same thing, and eventually that test should be
> increased to other packages too.

Let's be honest, developers are lazy. When under pressure to land
things, they go and look for the simplest way to get something done.

The problem has been that we didn't shout loud enough early enough that
there were to be "NO MORE STATE WORKERS", and what's more, making it a
priority to change the existing ones to api workers.

In case any one missed it, "NO MORE STATE WORKERS". Onyx will take the
dblogpruner and txnpruner as we added those, and Menno already mentioned
this.

Bugs have been filed for the five workers using *state.State directly,
and have been added to the tech-debt kanban board.

https://canonical.leankit.com/Boards/View/116651667#workflow-view

Tim

-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: workers using *state.State

2015-09-08 Thread Horacio Duran 
There is lazy and there is also "I just based in that other worker" which
happens, I am the proud parent of statushistorypruner and a rewrite is
underway too, sorry.

On Tuesday, September 8, 2015, Tim Penhey  wrote:

> On 09/09/15 11:22, Andrew Wilkins wrote:
> > On Wed, Sep 9, 2015 at 6:14 AM Ian Booth  
> > <mailto:ian.bo...@canonical.com >> wrote:
> >
> > Those workers below aren't the only ones. There's also minunits and
> > peergrouper
> > workers.
> >
> > No-one does these things on purpose. Just last week I caught and
> > rejected a pull
> > request to introduce a new worker depending on state directly.
> > People make
> > mistakes. Perhaps we should introduce a test which fails if state is
> > imported
> > into any worker code. We have similar tests already for other
> > forbidden imports.
> >
> >
> > +1. I was thinking the same thing, and eventually that test should be
> > increased to other packages too.
>
> Let's be honest, developers are lazy. When under pressure to land
> things, they go and look for the simplest way to get something done.
>
> The problem has been that we didn't shout loud enough early enough that
> there were to be "NO MORE STATE WORKERS", and what's more, making it a
> priority to change the existing ones to api workers.
>
> In case any one missed it, "NO MORE STATE WORKERS". Onyx will take the
> dblogpruner and txnpruner as we added those, and Menno already mentioned
> this.
>
> Bugs have been filed for the five workers using *state.State directly,
> and have been added to the tech-debt kanban board.
>
> https://canonical.leankit.com/Boards/View/116651667#workflow-view
>
> Tim
>
> --
> Juju-dev mailing list
> Juju-dev@lists.ubuntu.com 
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>
-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: workers using *state.State

2015-09-08 Thread Tim Penhey 
On 09/09/15 12:36, Horacio Duran wrote:
> There is lazy and there is also "I just based in that other worker"
> which happens, I am the proud parent of statushistorypruner and a
> rewrite is underway too, sorry.

Don't get me wrong, lazy developers are generally good. We try to find
the simplest thing that will work.

Tim


-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: workers using *state.State

2015-09-08 Thread Menno Smits 
You missed another worker that needs updating: envWorkerManager. Its use of
*state.State is a little less obvious.

Ticket and card added: https://bugs.launchpad.net/juju-core/+bug/1493606

On 9 September 2015 at 12:43, Tim Penhey  wrote:

> On 09/09/15 12:36, Horacio Duran wrote:
> > There is lazy and there is also "I just based in that other worker"
> > which happens, I am the proud parent of statushistorypruner and a
> > rewrite is underway too, sorry.
>
> Don't get me wrong, lazy developers are generally good. We try to find
> the simplest thing that will work.
>
> Tim
>
>
> --
> Juju-dev mailing list
> Juju-dev@lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>
-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: workers using *state.State

2015-09-08 Thread Tim Penhey 
On 09/09/15 12:47, Menno Smits wrote:
> You missed another worker that needs updating: envWorkerManager. Its use
> of *state.State is a little less obvious.

I had left that one off because I thought it only had the state instance
to pass on to other workers.

But I guess it does need updating, so thank you.

Tim


-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: workers using *state.State

2015-09-08 Thread Menno Smits 
On 9 September 2015 at 12:51, Tim Penhey  wrote:

> On 09/09/15 12:47, Menno Smits wrote:
> > You missed another worker that needs updating: envWorkerManager. Its use
> > of *state.State is a little less obvious.
>
> I had left that one off because I thought it only had the state instance
> to pass on to other workers.
>
> But I guess it does need updating, so thank you.
>
> Tim
>
>
It also uses the State itself to call State.WatchEnvironments (which will
need to be exposed via the API as part of the clean up).
-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: workers using *state.State

2015-09-09 Thread William Reade 
Hey all

I'm sorry for the unnecessarily harsh tone in the OP, which was absolutely
not called for. Thank you all for stepping up to address it regardless.

I would like to calmly state that `doc/architectural-overview.txt` -- more
than a year old -- does state that all workers, whatever agent they're in,
should use the API server; and that we were all actively working on
api-everywhere not *that* long before then, so I am genuinely
surprised/saddened that our institutional knowledge didn't catch and
correct any of these before I threw a tantrum in person.

FWIW, I think that envworkermanager's *current* situation is justifiable:
IIRC the machine agent still needs the state conn to set up the singular
worker, and I suspect there are other subtler threads in play too. That's
not to say it *should* be doing that; but it is just invoking the general
run-env-workers code that pre-existed in the single-env agent code, and we
need to do more agent work before we can fully extract that dependency.

Cheers
William

On Tue, Sep 8, 2015 at 9:12 AM, William Reade 
wrote:

> People keep writing them, in defiance of sane layering and explicit
> instructions, for the most embarrassingly trivial tasks
> (statushistorypruner? dblogpruner? txnpruner? *all* of those can and should
> pass through a simple api facade, not just dance off to play with the
> direct-db-access fairies.)
>
> There is no justification for *any* of those things to see a *state.State,
> and I'm going to start treating new workers that violate layering this way
> as deliberate sabotage attempts. Leads who have overseen the introduction
> of those workers, sort it out.
>
-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: workers using *state.State

2015-09-10 Thread Dimiter Naydenov 
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

FWIW in Sapphire we did spend quite some time in the last few months
to migrate the "addresser", "cleaner", and "instancepoller" workers to
use the API instead of state.State directly.

Dimiter

On  9.09.2015 11:42, William Reade wrote:
> Hey all
> 
> I'm sorry for the unnecessarily harsh tone in the OP, which was 
> absolutely not called for. Thank you all for stepping up to address
> it regardless.
> 
> I would like to calmly state that `doc/architectural-overview.txt`
> -- more than a year old -- does state that all workers, whatever
> agent they're in, should use the API server; and that we were all
> actively working on api-everywhere not *that* long before then, so
> I am genuinely surprised/saddened that our institutional knowledge
> didn't catch and correct any of these before I threw a tantrum in
> person.
> 
> FWIW, I think that envworkermanager's *current* situation is 
> justifiable: IIRC the machine agent still needs the state conn to
> set up the singular worker, and I suspect there are other subtler
> threads in play too. That's not to say it *should* be doing that;
> but it is just invoking the general run-env-workers code that
> pre-existed in the single-env agent code, and we need to do more
> agent work before we can fully extract that dependency.
> 
> Cheers William
> 
> On Tue, Sep 8, 2015 at 9:12 AM, William Reade 
> mailto:william.re...@canonical.com>>
> wrote:
> 
> People keep writing them, in defiance of sane layering and
> explicit instructions, for the most embarrassingly trivial tasks 
> (statushistorypruner? dblogpruner? txnpruner? *all* of those can
> and should pass through a simple api facade, not just dance off to
> play with the direct-db-access fairies.)
> 
> There is no justification for *any* of those things to see a 
> *state.State, and I'm going to start treating new workers that 
> violate layering this way as deliberate sabotage attempts. Leads
> who have overseen the introduction of those workers, sort it out.
> 
> 
> 
> 


- -- 
Dimiter Naydenov 
Juju Core Sapphire team 
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.22 (GNU/Linux)

iQEcBAEBAgAGBQJV8XCdAAoJENzxV2TbLzHwdfgH/Au/6JBrrkr8t4LV5w8eebZN
3OUAojGL5egoK8wBXOmS4LYJeZ4FZWipkHVexdcmTOeRvER1x4hV/kFL18rrnPbw
sqoO0CbW+dF2byRigvelYMMgiZiaGcDALNVt5v2/gHAfj43S4RpfniNYs/nOvaeN
yoVaWTyn7zR9cKbSULZoq7s61NSyuMMiKPTb7gDKu6MSBRjqtM+OPNKOQUYZpgOp
UeQyAw2NYE9vG0PUgPyaZevaFzwTVae0Tcu3QoRZzqoU5vBiYxXyGA+BAr3EXo6e
ksYWbF/2NP+UZR67OPl6zqKz7vgpWCXuCbX+rU+xOh+34Ud6jOUPXKCdPyvCA7I=
=J1s1
-END PGP SIGNATURE-

-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev