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

Federation rewrite with diaspora_federation gem #6873

Merged
merged 101 commits into from Jun 26, 2016

Conversation

@SuperTux88
Copy link
Member

@SuperTux88 SuperTux88 commented Jun 19, 2016

This is the last step of #5114. I rewrote the whole incoming and outgoing federation code to use the new federation-gem.

This PR currently uses the latest commit on the develop-branch of the federation gem. I would release the 0.1.0 version and update the PR after this PR was reviewed and tested successfully. Also I suggest to move the gem to the diaspora-organization after this PR is merged.

This PR also includes a complete rewrite of the notification-system, to fix the "sometimes no, sometimes one and sometimes two notification mails"-problem.

The following issues are fixed with this PR: #4727, #4697, #3666, #6579 (and many more bugs, where I didn't find an issue)

I think that #3519 also can be closed after this is merged, because this code doesn't exist anymore.

Note: The migration to cleanup participation (04c8329) can cause problems on big mysql databases if they have a innodb_data_file_path with max:. But this is not enabled by default on mysql (but somehow was on my local test-database). Should we add a note to the Changelog/Update-Notes for that? (see: http://stackoverflow.com/questions/21499169/innodb-delete-table-is-full)

Maybe someone can test the migrations with a bigger postgresql database? I only have a small test-DB (so the syntax should be OK), but I don't know if they cause trouble on bigger DBs (edge cases on old installations). I tested the migrations on a backup of my production mysql-database, so mysql should be OK.

I didn't write migrations to cleanup the author_signature of Like, Comment and PollParticipation, because @CSammy wants to make more database-optimizations and said that he can cleanup this then. Also the author_signature column of Message can be dropped completely after https://github.com/SuperTux88/diaspora_federation/issues/36 is done, so I didn't touch it now.

@cmrd-senya: maybe you can test this PR with your federation-tests?


(This part is not part of the initial PR text and gets abused as a ToDo list.)

ToDos before merging:

  • Move SuperTux88/diaspora_federation to diaspora/diaspora_federation
  • Verify migration compatibility on PostgreSQL
  • Manual testing of all the things
  • Second round of diaspora_federation gem reviewing
  • Release v0.1.0 of diaspora_federation gem

ToDos after merging:

  • Work through all federation related issues, close them or move them into the federation repository if applicable
  • Work through all notification related issues and see if those are fixed
  • Create metabug for tracking database optimizations (actually, a milestone and single issues would work even better)
  • Add a note regarding "full" MySQL tables into the upgrade notes.
@@ -1,9 +1,9 @@
.media.stream_element{:data=>{:guid => note.id, :type => (Notification.types.key(note.type) || '') }, :class => (note.unread ? 'unread' : 'read')}
.unread-toggle.pull-right
%i.entypo-eye{title: (note.unread ? t("notifications.index.mark_read") : t("notifications.index.mark_unread"))}
- if note.type == "Notifications::StartedSharing" && contact = current_user.contact_for(note.effective_target)
- if note.type == "Notifications::StartedSharing" && contact = current_user.contact_for(note.target)

This comment has been minimized.

@diaspora-code-review

diaspora-code-review Jun 19, 2016

Assignment in condition - you probably meant to use ==.

This comment has been minimized.

@denschub

denschub Jun 19, 2016
Member

Ignore.

class SendBase < Base
sidekiq_options queue: :http, retry: 0

MAX_RETRIES = (rt = AppConfig.environment.sidekiq.retry.get) ? rt.to_i : 0

This comment has been minimized.

@diaspora-code-review

diaspora-code-review Jun 19, 2016

Assignment in condition - you probably meant to use ==.

This comment has been minimized.

@denschub

denschub Jun 19, 2016
Member

to_i returns 0 if an invalid string/nil is provided:

MAX_RETRIES = AppConfig.environment.sidekiq.retry.get.to_i
module Diaspora
module Federation
module Entities
def self.build(entity)

This comment has been minimized.

@diaspora-code-review

diaspora-code-review Jun 19, 2016

Cyclomatic complexity for build is too high. [14/6]

This comment has been minimized.

@denschub

denschub Jun 19, 2016
Member

Yes, @diaspora-code-review, you are perfectly right, the cyclomatic complexity is way too high for a normal function. But dude, take a look at the source! It's simply calling some function depending on the incoming objects, mate. That's not an issue! I am sure you can be happy with that, can't you?

Oh, wait, I am talking to a bot. It's going down.

This comment has been minimized.

@SuperTux88

SuperTux88 Jun 20, 2016
Author Member

fixed 😺

@denschub denschub added this to the 0.6.0.0 milestone Jun 19, 2016
@@ -136,7 +136,6 @@ gem "leaflet-rails", "0.7.7"
gem "nokogiri", "1.6.8"
gem "redcarpet", "3.3.4"
gem "twitter-text", "1.13.4"
gem "roxml", "3.1.6"

This comment has been minimized.

@denschub

denschub Jun 19, 2016
Member

🎉 🎈 🍰 ❤️

[person]
end

# creates or updates a contact with active sharing flag. Returns nil if allready sharing.

This comment has been minimized.

@denschub

denschub Jun 19, 2016
Member

Nit: already.


return if contact.sharing

contact.tap do |contact|

This comment has been minimized.

@denschub

denschub Jun 19, 2016
Member

You like .tap, don't you? :) However, I am not too sure if it adds anything here besides looking fancy. Not using tap and returning the object without tap actually is a line shorter and looks less complicated:

contact.sharing = true
contact.save!
contact

Also, if you just update a single attribute,

contact.update(sharing: true)
contact

Would work, too.

Edit: actually, you do not need to return the contact. My point is still valid, though.

This comment has been minimized.

@SuperTux88

SuperTux88 Jun 19, 2016
Author Member

yes, I think the solution with update is better, I'll change that.

@SuperTux88 SuperTux88 force-pushed the SuperTux88:federation-gem-salmon branch from ec8ae07 to 8b3f451 Jun 19, 2016
when DiasporaFederation::Entities::Retraction
Diaspora::Federation::Receive.retraction(entity, recipient_id)
else
persisted = case entity

This comment has been minimized.

@denschub

denschub Jun 19, 2016
Member

Note to myself: think about if/how we can make this prettier.

This comment has been minimized.

@jhass

jhass Jun 19, 2016
Member

A hash, moving it out of sight of the glue code and public_send is probably acceptable here.

persisted = Diaspora::Federation::Receive.perform(entity)
# ...
def self.perform(entity)
  receiver = ENTITY_RECEIVERS.fetch(entity.class) { raise ... }
  public_send(receiver, entity)
end
pod.status = :no_errors unless Pod.statuses[pod.status] == Pod.statuses[:version_failed]
else
pod.status = :http_failed
pod.error = "FederationError: HTTP-Status-Code was: #{status}"

This comment has been minimized.

@denschub

denschub Jun 19, 2016
Member

Nit: English does not use hyphens to combine words

- HTTP-Status-Code
+ HTTP status code
class HttpMulti < Base
def perform(_user_id, _enc_object_xml, _person_ids, _retry_count=0)
class SendPrivate < Base
def perform(_sender_id, _obj_str, _targets, _retry_count=0)

This comment has been minimized.

@denschub

denschub Jun 19, 2016
Member

What do you think about def perform(*_args)? This would reduce the amount of changes we need to do if we ever change the signature of this function and we have other specs to check if the functions are called correctly.


def self.contact(entity)
recipient = Person.find_by(diaspora_handle: entity.recipient).owner
if entity.sharing.to_s == "true"

This comment has been minimized.

@denschub

denschub Jun 19, 2016
Member

Note to myself: No, @SuperTux88 is not crazy. The federation gem is only capable of passing strings at the moment and the conversion to a Boolean happens by AR. This is the only place where a check of that kind is needed, so the adjustment to the federation gem has a low priority, but will be done eventually.

@@ -1,3 +1,4 @@

This comment has been minimized.

@denschub

denschub Jun 19, 2016
Member

Oh hi there little newline! You look pretty out of place here, but I will help you finding your original destination!

expect(retry_delay).to be < 316
end

it "increases the retry time for every retry exponentially " do

This comment has been minimized.

@denschub

denschub Jun 19, 2016
Member

Nit: this is not an exponential increase.

@denschub
Copy link
Member

@denschub denschub commented Jun 19, 2016

So, before adding any more comments: Thank you for your work. I know that you spent over a year on the federation gem and on getting this first PR out. This is awesome and I guess it is fair to say that this is the biggest change for diaspora so far. It is very sad that, if everything goes well, no user will ever notice what was achieved here, but that's the fun of backend work, isn't it?

I finished a first round of review, mainly looking for obvious mistakes (which I did not find any) and style improvements. The migrations look fine to me, but I will run them on a copy of Geraspora's database (which, coincidentally, is a Postgres database) to make sure. Also, I do expect some more changes to come.

But... great work so far! ❤️

Finally, a note to everyone else: I know everyone is excited, but please keep the comments of this PR rather clean. Please add your heart and party emoticons to the PR, but please do not post any comments unless needed. If you have questions about a change or you want to suggest something but you are not sure if this is something that should be done, please contact a core team member via IRC. I do expect some technical discussions about some implementations in this PR and everything will be easier if we reduce the amount of off-topic comments. Thanks! <3

@SuperTux88 SuperTux88 force-pushed the SuperTux88:federation-gem-salmon branch 2 times, most recently from 1703e1b to f46d4a8 Jun 19, 2016
@denschub
Copy link
Member

@denschub denschub commented Jun 19, 2016

As for answering the questions in the initial PR text as well as keeping track of open tasks, I've added a list to the PR description.

@cmrd-senya
Copy link
Member

@cmrd-senya cmrd-senya commented Jun 19, 2016

@cmrd-senya: maybe you can test this PR with your federation-tests?

Yes, I'll test this during the next week.

current_user.disconnect(contact, :force => true)
end
contact = current_user.contact_for(person)
current_user.disconnect(contact) if contact

This comment has been minimized.

@jhass

jhass Jun 19, 2016
Member

Style question: Should start to make use of try for these kind of cases?

current_user.contact_for(person).try {|contact| current_user.disconnect(contact) }
def participant_handles
self.participants.map{|p| p.diaspora_handle}.join(";")
participants.map(&:diaspora_handle).join(";")

This comment has been minimized.

@jhass

jhass Jun 19, 2016
Member

How likely is it that we'll need the whole objects anyway if this is called? In other words could it be beneficial to pluck instead or are we going to load the whole objects anyway?

This comment has been minimized.

@SuperTux88

SuperTux88 Jun 19, 2016
Author Member

this method is called while dispatching, and the participants are probably already loaded, because he calls subscribers before he calls participant_handles.

self.conversation = parent
def conversation_guid=(guid)
cnv = Conversation.find_by(guid: guid)
self.conversation = cnv if cnv

This comment has been minimized.

@jhass

jhass Jun 19, 2016
Member

Similar micro optimization consideration, how likely is it that we'll need the whole Conversation object later if this was called? Else we could save the allocation with super if Conversation.exists?.

Silently ignoring the assignment feels worrying too, but probably out of scope here to fix. Might be worth a TODO comment nonetheless.

if author.local?
conversation.participants
else # for relaying, TODO: can be removed when messages are not relayed anymore
conversation.participants.select(&:remote?)

This comment has been minimized.

@jhass

jhass Jun 19, 2016
Member

Same again, is the whole collection loaded anyway? If not can we turn this into a DB query?

end
return_note.email_the_user(target, actor) if return_note
return_note
where(opts.merge!(recipient_id: recipient.id)).order("updated_at desc")

This comment has been minimized.

@jhass

jhass Jun 19, 2016
Member

Nit: uppercase DESC.

find_or_initialize_by(recipient: recipient, target: target, unread: true).tap do |notification|
notification.actors |= [actor]
# Explicitly touch the notification to update updated_at whenever new actor is inserted in notification.
notification.touch unless notification.new_record?

This comment has been minimized.

@jhass

jhass Jun 19, 2016
Member

Micro: We shouldn't need to save! if the record has no changes, just touch and we shouldn't need to touch if the record has changes, save! will do it:

if notification.new_record? || notification.changed?
  notification.save!
else
  notification.touch
end
def self.notify(comment, _recipient_user_ids)
actor = comment.author
commentable = comment.commentable
recipients = commentable.participations.map(&:author).select(&:local?) - [commentable.author, actor]

This comment has been minimized.

@jhass

jhass Jun 19, 2016
Member

Same question as before, is this only place where we force the collections here or could this be a joins().where().not()?

Post.where(:id => self.target_id).first
recipients.each do |recipient|
notification = concatenate_or_create(recipient.owner, commentable, actor)
notification.email_the_user(comment, actor) if notification

This comment has been minimized.

@jhass

jhass Jun 19, 2016
Member

Another .try { } candidate.

recipients.map(&:owner).each do |recipient|
message.increase_unread(recipient)
new(recipient: recipient).email_the_user(message, message.author)
end

This comment has been minimized.

@jhass

jhass Jun 19, 2016
Member

Same story about loading the entire collections, I'll skip any further ones for now.


def subscribers
subs = super
subs += resharers + participants if public?

This comment has been minimized.

@jhass

jhass Jun 19, 2016
Member

Do we own the object we get from the parent at this point? Array.concat could save a few array allocations here.

This comment has been minimized.

@SuperTux88

SuperTux88 Jun 19, 2016
Author Member

Do we own the object we get from the parent at this point?

Yes, the parent is only called if we own it.

This comment has been minimized.

@jhass

jhass Jun 19, 2016
Member

I mean does the parent hold a reference to it otherwise?

This comment has been minimized.

@SuperTux88

SuperTux88 Jun 19, 2016
Author Member

Sorry, I misunderstood you, no the parent doesn't hold a reference to it.

contact.update_attributes(:sharing => false)
else
contact.update_attributes(:receiving => false)
disconnect_contact(contact, :receiving, !contact.sharing)

This comment has been minimized.

@jhass

jhass Jun 19, 2016
Member

This call could benefit from keyword arguments:

disconnect_contact(contact, direction: :receiving, destroy: !contact.sharing)

def disconnect_contact(contact, direction:, destroy:)
@SuperTux88 SuperTux88 force-pushed the SuperTux88:federation-gem-salmon branch from da7c22e to 3b1d113 Jun 26, 2016
@denschub denschub merged commit 3b1d113 into diaspora:develop Jun 26, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
denschub added a commit that referenced this pull request Jun 26, 2016
Federation rewrite with diaspora_federation gem
@denschub
Copy link
Member

@denschub denschub commented Jun 26, 2016

Yihhhaaaa! 🎉

I've merged this so I'm able to test this on Geraspora. I hope this is stable enough for a dev-merge, but we can always file follow-ups.

Thanks again for your work here! ❤️

@Flaburgan
Copy link
Member

@Flaburgan Flaburgan commented Jun 26, 2016

the migrated database is 29,55% smaller than the old one

Was that migration done from a master database or from a develop one with #6586 already applied?

Awesome news! Big up for the new federation!

@denschub
Copy link
Member

@denschub denschub commented Jun 26, 2016

Yes, #6586 was already applied, so the size reduction for 0.5.0.0 -> 0.6.0.0 will be even more impressive.

@SuperTux88 SuperTux88 deleted the SuperTux88:federation-gem-salmon branch Jun 26, 2016
@Flaburgan
Copy link
Member

@Flaburgan Flaburgan commented Jun 28, 2016

I updated d-fr which now runs the last develop commits.
Before the pg_dump was 7972654124B
After it is 4846210865B
Here is the result of the db:migrate if you're interested about the time: https://pastebin.mozilla.org/8880142 (CleanupParticipations: migrated (2234.3895s))

Thank you very much!

SuperTux88 added a commit to SuperTux88/diaspora that referenced this pull request Aug 20, 2016
Before diaspora#6873 we deleted contacts when someone blocks a person, but we
didn't drop the notification for the started sharing event. In diaspora#6864
we try to get the contact for the notification, which is not there
anymore.

So we need to remove the notifications for the contacts that don't exit
anymore.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.