[O selection] server: Use BroadcastSessionsSelector in BroadcastSessionsManager

[yondonfu]

What does this pull request do? Explain your changes. (required)

This PR moves the current LIFO O selection logic from BroadcastSessionsManager to LIFOSelector which implements the BroadcastSessionsSelector interface. The goals of this refactor are:

• Allow new selection logic to be implemented separately from BroadcastSessionsManager in follow on PRs. New selection logic can be encapsulated in a new type that implements the BroadcastSessionsSelector interface
• Keep the existing LIFO selection logic in LIFOSelector to serve as an example of an alternate selection strategy once the new selection strategy is implemented in follow on PRs

Specific updates (required)

• Update BroadcastSessionsManager to accept the BroadcastSessionsSelector interface as a dependency
• Add LIFOSelector which implements BroadcastSessionsSelector

Note that LIFOSelector is not concurrency safe. The caller of LIFOSelector should implement locking i.e. BroadcastSessionsManager.

The BroadcastSessionsSelector API does not contain a method for removing sessions. The caller of the API should keep track of which sessions still exist. A session returned from Select() that no longer exists should be discarded and Select() should be called again. This design choice matches the existing design in BroadcastSessionsManager where sessMap is used to keep track of existing sessions and whenever a non-existent session is returned we re-select another session.

How did you test each of these updates (required)

Updated unit tests.

Does this pull request close any open issues?

Fixes #1228

Checklist:

• README and other documentation updated
• Node runs in OSX and devenv
• All tests in ./test.sh pass

[yondonfu]

Ready for full review.

[kyriediculous]

This overall looks good to me. Do you think it's worth the engineering effort to add unit tests for LIFOSelector or not , considering it's not actually going to be used once all PRs get merged.

[yondonfu]

Do you think it's worth the engineering effort to add unit tests for LIFOSelector or not , considering it's not actually going to be used once all PRs get merged.

I'm ok with not adding explicit unit tests for LIFOSelector right now for the reason that you mentioned and also because the BroadcastSessionsManager unit tests already largely test the behavior of LIFOSelector since each instance of BroadcastSessionsManager created in those tests use LIFOSelector and the tests include assertions on state updates for LIFOSelector.

[j0sh]

This looks pretty good.

On a somewhat related note re: code organization, how feasible would it be to move the BroadcastSession into core ? The broadcast.go file is pretty empty. Then most of the selection code could live within that as well - either directly within broadcast.go, in core or even its own pacakge, but that's my least favorite choice. We don't actually have to do this though, and I wonder if it actually makes sense for anything other than subjective taste. Keeping everything in server isn't really a problem anyway.

[yondonfu]

The broadcast.go file is pretty empty.

Do you mean core/broadcaster.go?

Moving BroadcastSession and the related selection code (i.e. BroadcastSessionsManager, BroadcastSessionsSelector, etc.) to core seems feasible, but one small downside I see is that if we also move the NewSessionsManager() constructor to core, then core would need to import server since NewSessionsManager() accepts streamParameters as a param which is defined in server.

Keeping everything in server isn't really a problem anyway.

I'm ok with keeping everything in server right now.

[j0sh]

Do you mean core/broadcaster.go?

Ah yep, that.

core would need to import server since NewSessionsManager() accepts streamParameters as a param which is defined in server.

streamParameters could also be defined within core/broadcaster.go as well - that seems OK, although all the fields would need to be made public. Which is also OK.

The only thing I really wish we had was a way to indicate that a struct such as streamParameters was strictly read-only, immutable product type without any attached methods. We have a few other data structures following this pattern, such as the BroadcastSession struct.

I'm ok with keeping everything in server right now.

Yeah, that's fine. Just want to keep that possibility in mind the next time someone complains about the length / scope of the files in server, and to make us a little more conscious of the issue so we aren't adding things later on which make this type of extraction more difficult. (Another implicit design outcome here is the fact server/broadcast.go has no LivepeerServer methods or instance methods outside of the BroadcastSessionsManager.)

I think this also exhibits part of the tension around code placement and package structure - which is pretty arbitrary once you get down to it. Put everything in a single file! (Kidding but only a little bit. Personally, I don't really have an issue with files that are thousands of lines long.)

Anyway all that's getting off topic for this PR. LGTM!