https://dev.arvados.org/https://dev.arvados.org/favicon.ico?15576888422014-04-16T15:11:08ZArvadosArvados - Idea #2608: Implement a websocket read-only event bus backed by the logs tablehttps://dev.arvados.org/issues/2608?journal_id=96752014-04-16T15:11:08ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Story points</strong> changed from <i>2.0</i> to <i>3.0</i></li></ul> Arvados - Idea #2608: Implement a websocket read-only event bus backed by the logs tablehttps://dev.arvados.org/issues/2608?journal_id=96802014-04-16T15:18:29ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Assigned To</strong> set to <i>Peter Amstutz</i></li><li><strong>Story points</strong> changed from <i>3.0</i> to <i>4.0</i></li></ul> Arvados - Idea #2608: Implement a websocket read-only event bus backed by the logs tablehttps://dev.arvados.org/issues/2608?journal_id=99892014-04-28T16:32:01ZTim Piercetwp@curoverse.com
<ul></ul><p>Code review comments. This is a first pass, I'll review more in-depth later, but this might help to start.</p>
Method summary comments that would be useful:
<ul>
<li><code>EventBus.push_events</code></li>
<li><code>EventBus.on_connect</code></li>
<li><code>RackSocket.initialize</code></li>
<li><code>RackSocket.call</code></li>
<li><code>RecordFilters.record_filters</code></li>
</ul>
<p>We should also have module comment docs for EventBus, RackSocket, LoadParam, RecordFilters etc. In general I'd like to see module comments that describe briefly what the module is for and what subsystem it supports, and method summary comments on every nontrivial method. Doesn't have to go into a huge amount of detail, just summarize what the method does, what it returns and what arguments it takes.</p>
<ul>
<li>services/api/lib/eventbus.rb
<ul>
<li>in alloc_filter_id: what are those parentheses doing? Does that actually serve a function?</li>
</ul>
</li>
<li>services/api/config/initializers/eventbus.rb
<ul>
<li>Recommend just checking <code>if ENV['ARVADOS_WEBSOCKETS']</code> and not testing for a specific value (this is what we do in the client with <code>ARVADOS_API_HOST_INSECURE</code>)</li>
</ul>
</li>
<li>services/api/db/migrate/20140423132913_add_object_owner_to_logs.rb
<ul>
<li>Are <code>:new_attributes</code> or <code>:old_attributes</code> set somewhere else in the migration? I don't see them mentioned anywhere else in the code. Or is this Rails reflection magic?</li>
</ul></li>
</ul> Arvados - Idea #2608: Implement a websocket read-only event bus backed by the logs tablehttps://dev.arvados.org/issues/2608?journal_id=100132014-04-29T11:18:04ZTim Piercetwp@curoverse.com
<ul></ul><p>More code review:</p>
<ul>
<li>services/api/config/application.default.yml
<ul>
<li>Do we need to provide people with guidance on setting <code>websocket_address</code> properly? Or can we set it to a sensible default? (I'm assuming <code>wss://localhost:3002/websocket</code> will not be a useful setting in production -- if it is, I'm misunderstanding something badly)</li>
</ul>
</li>
<li>services/api/app/middlewares/arvados_api_token.rb
<ul>
<li>I'm new to the use of "middlewares" here. After reading some of the Rack documentation, it sounds like the idea is: when a request comes in, it's run through each of the <code>config.middleware</code> classes in turn, running the <code>call</code> method on the request and forwarding the resulting request to the next middleware. Is that about right?</li>
<li>In turn, is that why <code>ArvadosApiToken.call</code> needs to clean up each time it's called -- because one <code>ArvadosApiToken</code> object in the middleware stack may be used to process many requests from different users?</li>
<li>Is it meaningful for an <code>ArvadosApiToken.call</code> to be invoked when @app is nil? Should that trigger an error or warning of some kind?</li>
</ul>
</li>
<li>services/api/app/models/arvados_model.rb
<ul>
<li>Per discussion in IRC, suggest moving the <code>user.is_admin</code> check outside the SQL query entirely (both to make the code clearer and possibly to short-circuit the need to do a query at all)</li>
</ul></li>
</ul> Arvados - Idea #2608: Implement a websocket read-only event bus backed by the logs tablehttps://dev.arvados.org/issues/2608?journal_id=100202014-04-29T13:52:47ZTim Piercetwp@curoverse.com
<ul></ul><ul>
<li>services/api/lib/eventbus.rb
<ul>
<li>I'm concerned that for the websocket to have its own <code>user</code> property updated from <code>current_user</code> is asking for trouble when <code>ws.user</code> doesn't get updated or cleared at the right time. Is this a necessary consequence of WebsocketController being subclassed from ApplicationController or something like that?</li>
<li>Should the <code>EventBus.on_connect</code> background thread reset @bgthread before it terminates? Right now it appears that nothing will ever set @bgthread to false once this thread has been started.</li>
</ul>
</li>
<li>services/api/test/integration/websocket_test.rb
<ul>
<li>Nice selection of tests, very thorough.</li>
<li>Does EventMachine guarantee that events will be emitted in the order they were received? If not, we should make sure that tests which generate multiple events can handle the results out-of-order (relevant to test "filter events", "multiple filters", etc)</li>
<li>test "connect, subscribe, filter events" should assert that there are no more events waiting to be processed.</li>
<li>test "connect, subscribe, multiple filters" should include at least one event not captured by any filter, and should assert that there are no more events waiting to be processed.</li>
<li>test "connect, subscribe, get event, unsubscribe": why does this test have a timer to close the websocket after 3 seconds?</li>
<li>test "connect, subscribe, get event, try to unsubscribe with bogus filter_id": just to be pedantic, let's ensure that the filter_id we use to unsubscribe is different from the one that was created -- e.g. <code>bogus_filter_id = d["filter_id"] + 1</code></li>
<li>test "connect, try subscribe too many filters": I hate magic constants. Can we replace "16" with <code>EventBus.MAX_FILTERS</code> here?</li>
</ul></li>
</ul> Arvados - Idea #2608: Implement a websocket read-only event bus backed by the logs tablehttps://dev.arvados.org/issues/2608?journal_id=100392014-04-30T11:24:40ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Assigned To</strong> changed from <i>Peter Amstutz</i> to <i>Tim Pierce</i></li></ul><ol>
<li>Added lots of code comments</li>
<li>Removed filter ids in favor of specifying the filter to remove directly.</li>
<li>new_attribute and old_attribute are part of the logs table from before, the migration is just extracting a from those hash values and putting it into a real queryable column</li>
<li>Added comments to application.yml.example</li>
<li>Yep, that's how Rack works</li>
<li>The functional tests now use ArvadosApiToken to set the current user environment, in that situation it doesn't really make sense to pass control on to another app. See test_helper.rb#authorize_with</li>
<li>Added is_admin check to #readable_by</li>
<li>current_user is only valid during connection setup, because we know ArvadosApiToken has already run to set up the environment, so ws.user is set to record which user is associated that websocket connection. The message, close, and channel events are processed on the EventMachine thread, which is separate from the Rack request thread and shared by all websocket connections, so we don't have the current_user environment set up.</li>
<li>EventMachine is a single-threaded event queue architecture, so events are definitely processed in the order they are received.</li>
<li>Added guard states to various test to assert no more events are processed</li>
<li>Added comment about the timer</li>
<li>Changed how subscribing works</li>
<li>No more magic constants.</li>
</ol> Arvados - Idea #2608: Implement a websocket read-only event bus backed by the logs tablehttps://dev.arvados.org/issues/2608?journal_id=100492014-04-30T14:35:19ZTim Piercetwp@curoverse.com
<ul><li><strong>Assigned To</strong> changed from <i>Tim Pierce</i> to <i>Peter Amstutz</i></li></ul><p>LGTM</p> Arvados - Idea #2608: Implement a websocket read-only event bus backed by the logs tablehttps://dev.arvados.org/issues/2608?journal_id=102652014-05-07T12:44:38ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>Resolved</i></li><li><strong>Story points</strong> deleted (<del><i>4.0</i></del>)</li></ul>