Alternative implementation of non-blocking

[guanyilun]

I was looking at issue #10 and thought maybe a better way to implement the non-blocking behavior is to move it to the base class so that you don't have to write it explicitly in every DataNodeServer? sorry if this is out of context here.

[BrianJKoopman]

Not out of context at all, I'm glad to see someone else take interest in sisock!

A few thoughts about the changes you've made:

• I like that this replaces two threads.deferToThread calls with one, probably the same thing could be accomplished by moving the _build_file_list call into _read_data_from_disk, adding a start and end argument to the latter.
• get_data is required by the API. It is how the grafana_http_json.py layer makes the call to the DataNodeServers to get the data when requested from Grafana, which is why get_data simply calls get_data_blocking, the blocking method is just a wrapper to implement the threading properly.
• get_data isn't necessarily always blocking, this is true in the case of the thermometry_server.py, which has the data cached in memory already (though this might change in this particular example if viewing data outside of the cache time that is written to disk is implemented directly in this DataNodeServer.)

This does bring up the good point of thinking a bit better about a more general implementation though. I'm usually willing to tolerate repeating myself once, but seeing as my next item was to implement reading .g3 files from disk for display in Grafana, likely some thought is due.

[mhasself]

I like this approach, and don't really see any problems. When sub-classing DataNodeServer, you now have the option of re-implementing get_data (if your code can run in the reactor) OR get_data_blocking (if your code does any blocking). Either way, the right thing happens when the external API runs get_data.

Minor point -- the class documentation should be updated to explain that. Currently get_data is still documented to say "This method must be overriden by child classes." (Which is also a typo... should be "overridden".)

Thanks for the contribution!

[BrianJKoopman]

I like this approach, and don't really see any problems. When sub-classing DataNodeServer, you now have the option of re-implementing get_data (if your code can run in the reactor) OR get_data_blocking (if your code does any blocking). Either way, the right thing happens when the external API runs get_data.

Right, sorry this wasn't clear to me when I looked at it. Ignore my 2nd and 3rd bullets. I'll test this with the data and check that things still work, update those docstrings, then likely merge.

[ahincks]

Either way, the right thing happens when the external API runs get_data.

I confess I'm not seeing how this happens automatically. Does the child class have to do something like get_data = get_data_blocking? Or does the user have to know to call one or the other? I must be missing something in the code ...

[ahincks]

And I should also say thanks, @guanyilun, for the pull request! Soon our project will list three contributors :-)

[BrianJKoopman]

Either way, the right thing happens when the external API runs get_data.

I confess I'm not seeing how this happens automatically. Does the child class have to do something like get_data = get_data_blocking? Or does the user have to know to call one or the other. I must be missing something in the code but am not seeing it ...

I think the idea is that if the child class needs get_data to block it overloads the get_data_blocking method, which get_data now calls. If it doesn't then the child class overloads get_data like usual.

[ahincks]

I think the idea is that if the child class needs get_data to block it overloads the get_data_blocking method, which get_data now calls. If it doesn't then the child class overloads get_data like usual.

Oh, yes, now I see it. Duh. Thanks. But now I have a couple of other comments:

Since the user only ever calls get_data(), should we not treat the blocking function as private in the pythonic way by renaming it _get_data_blocking()?

And in base.py, I would think that get_data() shouldn't do:

start = sisock_to_unix_time(start)
end = sisock_to_unix_time(end)

before calling get_data_blocking(), because the API for the latter implies that it is converting to UNIX time itself. Unless we change the API of get_data_blocking() such that start and stop must be pure UNIX times. Either solution is fine in my opinion.

[guanyilun]

And in base.py, I would think that get_data() shouldn't do:

start = sisock_to_unix_time(start)
end = sisock_to_unix_time(end)

before calling get_data_blocking(), because the API for the latter implies that it is converting to UNIX time itself. Unless we change the API of get_data_blocking() such that start and stop must be pure UNIX times. Either solution is fine in my opinion.

That's a good point. It makes the two methods not completely equivalent in term of inputs. I will remove them in the next commit.

[ahincks]

OK, thanks. Let's also rename get_data_blocking() to _get_data_blocking(), unless there is a good reason not to.

[guanyilun]

OK, thanks. Let's also rename get_data_blocking() to _get_data_blocking(), unless there is a good reason not to.

Yes, I have renamed it in the last commit too.

[BrianJKoopman]

Thanks for all the revisions @guanyilun! Sorry this took forever to merge.

[guanyilun]

@guanyilun Based on the commit message here it sounds like this catches up by merging in a commit that was made on master while this pull request was outstanding. This ends up making the history a bit less clear than it really is. I did try to figure out a way to fix this, but I ended up losing changes in each test I did, so I ended up just merging as is.

In the future I'd suggest rebasing on top of master rather than merging, as suggested in the SO Dev Guide.

@BrianJKoopman Thanks for merging and sorry about not following the guideline! I will keep that in mind next time.