sangoma / switchy

async FreeSWITCH cluster control
https://switchy.readthedocs.io/en/latest/
Mozilla Public License 2.0
69 stars 18 forks source link

Flask-like routing and cluster Service API #49

Closed goodboy closed 7 years ago

goodboy commented 7 years ago

In the spirit of ClueCon this adds a couple new major APIs/features as well as a bunch of fixes and docs.

The main goal here is to showcase switchy's capabilities as a FreeSWITCH cluster control library for building scalable carrier services. I had to dig quite a bit into the FreeSWITCH XML dialplan stuff to figure out how to design the API and ended up concluding that to be able to do sane async control from Python I'll most likely be moving the entire core to ascynio once #44 get's in. The whole callbacks / roll our own micro-twisted just isn't as sane (or cool).

I don't expect everyone to go over all the code in detail but I would appreciate a critique of the new feature code and particularly the docs:

Pinging @moises-silva @angvp @jmesquita @vodik @tsemczyszyn and @dtkerr because you all know a bit of both Python and FreeSWITCH.

Thanks!

goodboy commented 7 years ago

Oh and of course @ncorbic!

goodboy commented 7 years ago

Also just to clarify I don't expect you guys to code review much more then the code in:

DO NOT REVIEW THE .rst FILES instead look at a hosted version on our sphinx server:

idletea commented 7 years ago

So some feedback here on evals is that it might be nicer to not use string-based commands in the various calls. If your evals call used the __code__ attribute, if it exists, on the expression you could rewrite something like

def is_alive(self):
    return any(self.pool.evals('listener.is_alive()'))

as

def is_alive(self):
    def alive():
        listener.is_alive()
    return any(self.pool.evals(alive))

which I think is nicer personally.

I haven't looked at all of it, but if the only goal of evals is to let you call methods or get attributes from the listener or client it might also make more sense to rewrite evals to let you call it something like either of these:

def is_alive(self):
    return any(self.pool.evals(lambda listener: listener.is_alive()))

def is_alive(self):
    def slave_alive(listener):
        return listener.is_alive()
    return any(self.pool.evals(slave_alive))

Which can work since you can extract the name of arguments from a code object by looking at co_varnames

vodik commented 7 years ago

I think I'd agree with @dtkerr suggestion because I can't see how eval could be faster than a function call. Evaluating code should have more overhead, no?

Looking at your code, I see how and why you do the evals - to run the same snippet with different globals/locals, but I don't see how any of that is desirable over function calls...

Anyways, that is neither here nor there....


Weird inconsistencies in http://sphinxdocs.qa.sangoma.local/switchy/fsconfig.html

1470939399

Another random link in header: http://sphinxdocs.qa.sangoma.local/switchy/services.html

1470939685

That link probably best belongs in the body of the paragraph there, with some explanation what I'm supposed to find useful from it. Either I already understand FreeSWITCH dialplans or I don't - but I'm not writing XML so I don't see the direct connection. I assume its there to provide as documentation about available variables, but that could be better conveyed.

Same goes for the other links like "break on failure".

Glossary not in the TOC: http://sphinxdocs.qa.sangoma.local/switchy/glossary.html

Probably should be. Missing the TOC on the left of the page as well.

goodboy commented 7 years ago

Thanks @dtkerr @vodik. Regaring switchy.distribute.MultiEval.evals yes I agree that maybe it should be changed to a multi-func caller. That code is actually over a year old at this point so it's probably deserving of a separate issue for discussion. One thing I will say is that something like:

def is_alive(self):
    def alive():
        listener.is_alive()
    return any(self.pool.evals(alive))

has to lookup listener from somewhere. eval makes this simple by just handing it a namespace but invoking functions means probably building functools.partials or something similar.

Heck, let's use #50 to discuss it further.

Going back the review at hand...

Not a fan of how either paths are described. Putting it in the comment doesn't make it clear its the file you're supposed to edit. Putting it in italics puts the wrong emphasis on it. Also not a fan of the double ++/--. You're deviating too far away from standard diff notation.

@vodik I'd appreciate specific suggestions particularly for the former.

I assume its there to provide as documentation about available variables, but that could be better conveyed.

@vodik the point of the link is to cross reference with how you would access variables from the XML dp versus from the Python API. The reason it's in headers was to get them to show up in the TOC. That's also what all the other links to the XML dialplan wiki section are for; to give a direct comparison between Python and the XML dp.

I guess I was thinking that having the title as the link (or containing one) was just a way of avoiding putting it in the body again. For example I've seen it in the pandas docs but yeah I guess it's not common / a bit silly.

So as it stands I'm going to change the following:

vodik commented 7 years ago

Not a fan of how either paths are described. Putting it in the comment doesn't make it clear its the file you're supposed to edit. Putting it in italics puts the wrong emphasis on it.

@vodik I'd appreciate specific suggestions particularly for the former.

Put it above the code block in monospaced font above the block/and or integrate it into the paragraph.

@vodik the point of the link is to cross reference with how you would access variables from the XML dp versus from the Python API. The reason it's in headers was to get them to show up in the TOC. That's also what all the other links to the XML dialplan wiki section are for; to give a direct comparison between Python and the XML dp.

Wait, those links show up in the TOC by virtue of being linked from a header?

But still, its a semantic problem. Why would I click on the header just because its underlined? How does a reader know any of this outside of experimenting and clicking on random links essentially just because.

I get the point, but having a sentence saying "You can get more information from the FreeSWITCH documentation on here".

I guess I was thinking that having the title as the link (or containing one) was just a way of avoiding putting it in the body again. For example I've seen it in the pandas docs but yeah I guess it's not common / a bit silly.

Panda docs are different. Its a link to method call - which is just incidental. Its more likely they marked up the function to get consistent styling and a hyperlink is the side effect rather than the desired outcome.

I'm not really against links in the headers, but its a semantics thing, but you're doing them without considering semantics. For example, going through your document, I might establish "oh, all these hyperlinks in headers are just glossary links", and never realise that the ones near the bottom of the page are instead meant for additional documentation.

goodboy commented 7 years ago

@vodik everything should be addressed. Hope it's up to snuff :)

moises-silva commented 7 years ago

@tgoodlet Great work! looks really nice, I'm really looking forward to use this as soon as I can

goodboy commented 7 years ago

@moises-silva adjusted according to your suggestions. If you don't mind give it quick sanity check. Thanks!

goodboy commented 7 years ago

Alright unless there's any other suggestions on this I'm going to bring it in (@moises-silva @vodik).