Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support changefeeds in the web UI #2643

Closed
coffeemug opened this issue Jul 2, 2014 · 19 comments
Closed

Support changefeeds in the web UI #2643

coffeemug opened this issue Jul 2, 2014 · 19 comments
Assignees
Milestone

Comments

@coffeemug
Copy link
Contributor

There are a few reasons to do it:

  • We really should support all features in the web ui. It's a bad idea to lock some queries out.
  • It's really, really, really useful for demos.

We should figure out how to do this effectively before we ship 1.14.

/cc @neumino @mglukhovsky

@coffeemug coffeemug added this to the 1.14 milestone Jul 2, 2014
@neumino
Copy link
Member

neumino commented Jul 2, 2014

Ping @Tryneus and @mlucy
I think one of you mentioned some limitations with feeds and the HTTP connections.

Can I have change feeds over an HTTP connection?

@mglukhovsky
Copy link
Member

I would like to avoid us having to implement a server-side component over something like Socket.IO to allow following changefeeds -- if we can do it over an HTTP connection straight from the driver in the web UI, that would be amazing.

@mlucy
Copy link
Member

mlucy commented Jul 3, 2014

Currently changefeeds are disabled over HTTP because some internal asserts cause a crash when the HTTP connection times out while a changefeed is open (we have to block to unsubscribe from the changefeed, and the timer function doesn't like that). This would be relatively easy to fix.

@neumino
Copy link
Member

neumino commented Jul 3, 2014

HTTP request will also eventually timeout, so I'm not sure we can keep a feed for more than a few minutes too.

@mlucy
Copy link
Member

mlucy commented Jul 6, 2014

This is assigned to @neumino right now, but he can't really start prototyping until someone on the server re-enables changefeeds and fixes the old crashing bug. I or @Tryneus could do that (or Graham could with a little help -- it would be a good way to get a surface-level introduction to coroutines, blocking a coroutine, send, etc).

@mlucy
Copy link
Member

mlucy commented Jul 6, 2014

Assigning to myself, but I might not get to it immediately, so feel free to steal it.

@mlucy mlucy self-assigned this Jul 6, 2014
@coffeemug coffeemug modified the milestones: 1.15, 1.16 Sep 4, 2014
@AtnNn AtnNn assigned AtnNn and unassigned mlucy Oct 22, 2014
@AtnNn
Copy link
Member

AtnNn commented Oct 22, 2014

@mlucy @danielmewes I'm stealing this issue.

@danielmewes
Copy link
Member

👍

@coffeemug
Copy link
Contributor Author

@AtnNn -- could you write up a quick summary on how the UI will work? Me and @mglukhovsky care about it a lot and I have a bunch of opinions. In particular:

  • When the feed is on a range (e.g. t.changes()) I think we should show a gmail-like notification bar that says "More changes are available" with a link to see new changes.
  • When the feed is on a rage, I'm not entirely sure what to do. Perhaps we can update the document and do a quick background flash with a fadeout on the web ui. Or may be show a message saying "the document has changed" with a link to see the change.

Do you have an idea of how you'd like to do this?

@AtnNn
Copy link
Member

AtnNn commented Oct 24, 2014

Currently my favourite idea is to have change feeds displayed like cursors, but with the newest row at the top and updated automatically, with a pause button. I don't think it should be different if the feed is on a range.

Updating documents with a quick background flash sounds like a great idea for regular queries. There could be a configuration option that enables it. The data explorer could show the results of query and run query.changes() in the background, updating the results live. If we implemented #523 there are a lot of other cool things we could do, such as allow editing the results directly.

I will get to know the data explorer code more intimately this week. I will have a more complete proposal next week.

@coffeemug
Copy link
Contributor Author

Hmm, interesting. It's kind of hard to envision in the mind's eye what it's going to be like/which approach is better until we play with it, so I'm really looking forward to that!

@AtnNn
Copy link
Member

AtnNn commented Oct 28, 2014

Here's my plan.

  1. Move the logic for handling query results out of the Container class and into a new QueryResult class. The QueryResult class will have a Backbone.Events mixin that it uses to trigger events for errors or new cursor rows. This is in review and in branch atnnn/2643. This change does not affect the ResultView class.
  2. Refactor the monolithic ResultView and SharedResultView classes into TreeView, TableView, RawView and ProfileView classes, a TabbedResultView that wraps those four classes, and a ResultView as a parent of these five classes, with the common code. This should be a straightforward change and will make the following steps, and future enhancements, easier to implement.
  3. The logic in Container for taking changes out of the QueryResult object and handing them to the ResultView will be removed. Container will just pass the QueryResult object to the active ResultView.
  4. It should then be easy to adapt each ResultView to listen for new rows itself and display each group of 40 rows as they are received instead of all at once.
  5. The UI should then be in a very usable state. We can test it and see how well it works and what it is missing.

@neumino Do you have any opinion or advice? It will take a lot more time before I get to know the code as well as you do.

@neumino
Copy link
Member

neumino commented Oct 28, 2014

@AtnNn -- The class SharedResultView was the begining of a refactoring for the data browser (#1592) or to add a data explorer per table view. It doesn't really make sense to keep it now, so removing it is a good idea.

Breaking ResultView in multiple classes make sense, but there are a few tricks:

  • The links to switch view is currently in ResultView. You would have to take the links out of it or make pass them their container (a view has access by default to only @$el).
  • You would have to make QueryResult keep a copy of the displayed results (they are currently stored in ResultView) to be able to render a new TreeView/TableView/etc.

The third bullet point is a good idea. I'm currently passing args which is an object with multiple fields to ResultView.render_result, and it make more sense to have a proper class for that.

Overall, it seems like a good idea to me, the current classes are pretty heavy (sorry :)).
Let me know if you have more questions, I'm also on IRC if you need some help.

@neumino
Copy link
Member

neumino commented Oct 28, 2014

Ah also, if you want to quickly implement feeds without refactoring things, one way to do it is:

  • When you get back the response from the server, and when it is a feed, you can immediately call result_view.render_result()
  • Everytime you get new results from the feed, you can just call result_view.add_result (to implement) that would just add rows in @results and display a note "More data available"
  • Call result_view.render_result() every time the user click on "More data available"

@AtnNn
Copy link
Member

AtnNn commented Oct 28, 2014

Thanks @neumino.

I did at first implement something akin to result_view.add_result, but it left the code in a very tightly coupled state. Tewari analysis led me to the set of refactorings I described above.

@neumino
Copy link
Member

neumino commented Oct 28, 2014

One more thing @AtnNn, the data explorer used to support multiple queries, but we removed this at some point (I actually don't exactly remember when or why -- it was for UX reason, but also for some technical concerns related to the HTTP connections).
But the logic to handle asynchronous queries is still there (this is why we generate callbacks and check for an id_execution. You can remove this if you want to make the code lighter.

@AtnNn
Copy link
Member

AtnNn commented Oct 28, 2014

I have already removed the generated callbacks and the id_execution. They seemed superfluous but I wasn't entirely certain. Thanks for the explanation.

@AtnNn
Copy link
Member

AtnNn commented Jan 21, 2015

Merged into next and v1.16.x as 752c755.

@AtnNn AtnNn closed this as completed Jan 21, 2015
@danielmewes
Copy link
Member

Super awesome 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants