Landscape Client

Merge lp://staging/~radix/landscape-client/reactor-refactor-2 into lp://staging/~landscape/landscape-client/trunk

  1. reactor-refactor-2
  2. Merge into trunk
Proposed by Christopher Armstrong
Status: Rejected
Rejected by: David Britton
Proposed branch: lp://staging/~radix/landscape-client/reactor-refactor-2
Merge into: lp://staging/~landscape/landscape-client/trunk
Diff against target: 515 lines (+94/-53)
19 files modified
landscape/amp.py (+3/-2)
landscape/broker/client.py (+2/-1)
landscape/broker/service.py (+1/-1)
landscape/broker/tests/helpers.py (+7/-4)
landscape/broker/tests/test_service.py (+2/-1)
landscape/manager/manager.py (+2/-3)
landscape/manager/service.py (+3/-3)
landscape/manager/tests/test_service.py (+2/-1)
landscape/manager/usermanager.py (+2/-2)
landscape/monitor/monitor.py (+3/-4)
landscape/monitor/service.py (+4/-3)
landscape/monitor/tests/test_monitor.py (+2/-1)
landscape/monitor/tests/test_service.py (+2/-1)
landscape/monitor/tests/test_usermonitor.py (+2/-1)
landscape/monitor/usermonitor.py (+2/-1)
landscape/reactor.py (+31/-14)
landscape/service.py (+4/-0)
landscape/tests/helpers.py (+7/-3)
landscape/tests/test_amp.py (+13/-7)
To merge this branch: bzr merge lp://staging/~radix/landscape-client/reactor-refactor-2
Reviewer Review Type Date Requested Status
Alberto Donato Approve
Free Ekanayaka (community) Approve
Review via email: mp+164050@code.staging.launchpad.net

Description of the change

ComponentPublisher now takes a Twisted reactor (i.e., twisted.internet.reactor) as an argument instead of a LandscapeReactor. A bunch of stuff needed to change for this to work. Most notably, the Twisted reactor needed to be passed throughout the codebase correctly, both in the real deployment code and in the tests. Still, the diff isn't huge (~500 lines).

To post a comment you must log in.
Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

Hey Chris the changes look fine, I just have a few questions:

1) Do you think it's feasible to complete the real-reactor migration before you leave? It'd be good to not have 2 reactors in those __init__ constructors for too long.

2) Afaict the only feature of LandscapeReactor which is kind of missing in the real Twisted reactor is event handling (actually the Twisted reactor does have it, but I'm not sure it has the same semantics or if it's a good idea to use it at all). What are you planning to do for that? Have a separate "events" parameter that you pass to the __init__ constructors of classes that require it? This would be a tad unfortunate because we grow the list of dependencies, but maybe the tradeoff is worth.

review: Needs Information
Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

Chris and I discussed 1) on IRC, and decided that it's okay to grow a new events argument where needed (hopefully not in many places, and could be an incentive in reducing its use).

I'm basically +1, but I'm a little worried about 2), since there's some risk that this reactor remains half-done (of course the rest of the team could take it to completion tho).

Maybe you could commit it to a separate "buffering" branch that we merge only when complete.

review: Approve
lp://staging/~radix/landscape-client/reactor-refactor-2 updated
682. By Christopher Armstrong

merge from trunk

Revision history for this message
Alberto Donato (ack) wrote :

Looks good, +1

I agree it'd be nice to keep a separate branch until the transition is completed.

review: Approve
Revision history for this message
Christopher Armstrong (radix) wrote :

I'm heading off, guys. I didn't manage to get very far with reactor-refactor-3 (it ended up becoming a behemoth of a change, as it usually does... everything is interconnected!), but if anyone else feels inspired, the branch is there.

Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

Thanks for kicking off this, take care!

Revision history for this message
David Britton (dpb) wrote :

Rejected the MP to get it out of the tarmac queue

Unmerged revisions

682. By Christopher Armstrong

merge from trunk

681. By Christopher Armstrong

lint

680. By Christopher Armstrong

revert an unrelated part of the remove-reactor refactoring (still needs to happen, but not in this branch)

679. By Christopher Armstrong

holy crap... all the tests pass.

678. By Christopher Armstrong

- get rid of a pointless/untested KeyboardInterrupt handling clause which
  added a dependency of the event handling code on the reactor
- add an EventSystem class which only provides event logic, not reactor
  interfaces

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: