notes pub/sub via faye #6822

Closed
wants to merge 1 commit into
from

Conversation

4 participants
@skv-headless
Contributor

skv-headless commented Apr 18, 2014

Replace notes polling to pub/sub via faye.
The main reason for that is high load which generated by polling.
At our installation 80%-90% requests are notes requests.
Also with pub/sub comments appear immediately.

@@ -166,6 +166,9 @@ gem "gitlab_emoji", "~> 0.0.1.1"
gem "gon", '~> 5.0.0'
gem 'nprogress-rails'
+gem "private_pub"
+gem 'thin'

This comment has been minimized.

@jvanbaarsen

jvanbaarsen Apr 19, 2014

Contributor

Why did you move thin out of the development group?

@jvanbaarsen

jvanbaarsen Apr 19, 2014

Contributor

Why did you move thin out of the development group?

This comment has been minimized.

@skv-headless

skv-headless Apr 19, 2014

Contributor

faye use it.

@skv-headless

skv-headless Apr 19, 2014

Contributor

faye use it.

@@ -1,2 +1,3 @@
web: bundle exec unicorn_rails -p ${PORT:="3000"} -E ${RAILS_ENV:="development"} -c ${UNICORN_CONFIG:="config/unicorn.rb"}
worker: bundle exec sidekiq -q post_receive,mailer,system_hook,project_web_hook,common,default,gitlab_shell
+faye: bundle exec rackup private_pub.ru -s thin -E production

This comment has been minimized.

@jvanbaarsen

jvanbaarsen Apr 19, 2014

Contributor

This means that when i run foreman start for development purposes, faye will start in production. Would it be possible for it to take over the rails_env?

@jvanbaarsen

jvanbaarsen Apr 19, 2014

Contributor

This means that when i run foreman start for development purposes, faye will start in production. Would it be possible for it to take over the rails_env?

This comment has been minimized.

@skv-headless

skv-headless Apr 19, 2014

Contributor

Faye doesn't work with -E development actually I don't why. But It works in all environment.

@skv-headless

skv-headless Apr 19, 2014

Contributor

Faye doesn't work with -E development actually I don't why. But It works in all environment.

@@ -0,0 +1,10 @@
+development:
+ server: "http://localhost:9292/faye"
+ secret_token: "secret"

This comment has been minimized.

@jvanbaarsen

jvanbaarsen Apr 19, 2014

Contributor

I think it would be best to document somewhere that people have to change those secrets. Or maybe better, create an: config/private_pub.xml.example and let people copy it.

@jvanbaarsen

jvanbaarsen Apr 19, 2014

Contributor

I think it would be best to document somewhere that people have to change those secrets. Or maybe better, create an: config/private_pub.xml.example and let people copy it.

@jvanbaarsen

This comment has been minimized.

Show comment
Hide comment
@jvanbaarsen

jvanbaarsen Apr 19, 2014

Contributor

Is there a request on the feature request forum that is similar to this? If so, can you make a comment with a link to it? Please be aware that new functionality that is not marked accepting merge/pull requests on the forum might not make it into GitLab. You might be asked to make changes and even after implementing them your feature might still be declined. If you want to reduce the chance of this happening please have a discussion in the forum first.

Contributor

jvanbaarsen commented Apr 19, 2014

Is there a request on the feature request forum that is similar to this? If so, can you make a comment with a link to it? Please be aware that new functionality that is not marked accepting merge/pull requests on the forum might not make it into GitLab. You might be asked to make changes and even after implementing them your feature might still be declined. If you want to reduce the chance of this happening please have a discussion in the forum first.

@skv-headless

This comment has been minimized.

Show comment
Hide comment
@skv-headless

skv-headless Apr 19, 2014

Contributor

I hope you will be exited about this PR. I'm not sure that it's a feature, rather than performance optimization, I wrote about number of requests. I know that there is some work, which I can't finish without core team help, but I will not continue to work with that until you decide are you need it.

Contributor

skv-headless commented Apr 19, 2014

I hope you will be exited about this PR. I'm not sure that it's a feature, rather than performance optimization, I wrote about number of requests. I know that there is some work, which I can't finish without core team help, but I will not continue to work with that until you decide are you need it.

@jvanbaarsen

This comment has been minimized.

Show comment
Hide comment
@jvanbaarsen

jvanbaarsen Apr 20, 2014

Contributor

@randx / @dosire Can you decide about this feature please?

Contributor

jvanbaarsen commented Apr 20, 2014

@randx / @dosire Can you decide about this feature please?

@jvanbaarsen

This comment has been minimized.

Show comment
Hide comment
@jvanbaarsen

jvanbaarsen Apr 20, 2014

Contributor

@skv-headless Thanks for your contribution though!

Contributor

jvanbaarsen commented Apr 20, 2014

@skv-headless Thanks for your contribution though!

@dosire

This comment has been minimized.

Show comment
Hide comment
@dosire

dosire Apr 20, 2014

Member

It would be awesome to replace this with pub/sub. On GitLab.com 86% of the time is spend on NotesController#Index. But @randx will have to look into this MR.

Member

dosire commented Apr 20, 2014

It would be awesome to replace this with pub/sub. On GitLab.com 86% of the time is spend on NotesController#Index. But @randx will have to look into this MR.

@dzaporozhets

This comment has been minimized.

Show comment
Hide comment
@dzaporozhets

dzaporozhets Apr 22, 2014

Member

Why not mount faye inside rails app?

Member

dzaporozhets commented Apr 22, 2014

Why not mount faye inside rails app?

@skv-headless

This comment has been minimized.

Show comment
Hide comment
@skv-headless

skv-headless Apr 22, 2014

Contributor

Unicorn can't handle persistent connections.

Contributor

skv-headless commented Apr 22, 2014

Unicorn can't handle persistent connections.

@dzaporozhets

This comment has been minimized.

Show comment
Hide comment
@dzaporozhets

dzaporozhets Apr 22, 2014

Member

adding one more web server process is also not an option. Running sidekiq, unicorn, thin will increase amount of memory to start app and also make things more complicated. I am ok with faye but only mounted inside rails app so we can start only one webserver. If unicorn is not working - please suggest alternative web server.

Member

dzaporozhets commented Apr 22, 2014

adding one more web server process is also not an option. Running sidekiq, unicorn, thin will increase amount of memory to start app and also make things more complicated. I am ok with faye but only mounted inside rails app so we can start only one webserver. If unicorn is not working - please suggest alternative web server.

@dzaporozhets

This comment has been minimized.

Show comment
Hide comment
@dzaporozhets

dzaporozhets Apr 22, 2014

Member

Or we can just impove existing comment loading logic. Ex. ask server only for new comments

Member

dzaporozhets commented Apr 22, 2014

Or we can just impove existing comment loading logic. Ex. ask server only for new comments

@dosire

This comment has been minimized.

Show comment
Hide comment
@dosire

dosire Apr 22, 2014

Member

I would really like that, not implement pub/sub but query the server with the updated_at time of the most recent commit. This means the server doesn't have to go over all comments to see if something changed.

Member

dosire commented Apr 22, 2014

I would really like that, not implement pub/sub but query the server with the updated_at time of the most recent commit. This means the server doesn't have to go over all comments to see if something changed.

@jvanbaarsen

This comment has been minimized.

Show comment
Hide comment
@jvanbaarsen

jvanbaarsen Apr 22, 2014

Contributor

If that means we dont have to support another webserver, im all for that option :D

Contributor

jvanbaarsen commented Apr 22, 2014

If that means we dont have to support another webserver, im all for that option :D

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