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.

Sign up for GitHub

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

Jump to bottom

Adds support for RabbitMQ messaging #153

Merged
merged 1 commit into from
Sep 28, 2017
Merged

Adds support for RabbitMQ messaging #153

merged 1 commit into from
Sep 28, 2017

Conversation

jrgriffiniii
Copy link
Contributor

@jrgriffiniii jrgriffiniii commented Aug 31, 2017 •
edited
Loading

Resolves #104 by adding support for RabbitMQ messaging for the creation, updates, and deletion of Figgy resources

In addition to the improvements identified by @tpendragon , the following must be addressed:

  • Rebase using the latest commits on master
  • Refactor the messaging methods in PlumChangeSetPersister as registered handlers
  • Increase the coverage and quality of the test suites
  • Implement handlers for after_save_commit and after_delete_commit

@jrgriffiniii jrgriffiniii force-pushed the issues-104-jrgriffiniii-fire-rabbitmq-events branch 6 times, most recently from 03a77e7 to e3749c2 Compare September 6, 2017 20:36
@jrgriffiniii jrgriffiniii changed the title [WIP] Adds support for RabbitMQ messaging Adds support for RabbitMQ messaging Sep 6, 2017
tpendragon
tpendragon requested changes Sep 11, 2017
View reviewed changes
app/change_set_persisters/plum_change_set_persister.rb Outdated
@@ -19,10 +19,15 @@ def save(change_set:)
end

def delete(change_set:)
if change_set.resource.is_a?(FileSet)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this to before_delete?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or even better, a method that before_delete calls.

app/change_set_persisters/plum_change_set_persister.rb Outdated
before_delete(change_set: change_set)
persister.delete(resource: change_set.resource).tap do
after_commit unless transaction?
end
messenger.record_updated(updated_resource) if updated_resource
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, can we move to after_commit?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And maybe we need like a...after_delete_commit

app/change_set_persisters/plum_change_set_persister.rb Outdated
@@ -69,6 +81,12 @@ def before_save(change_set:)

def after_save(change_set:, updated_resource:)
append(append_id: change_set.append_id, updated_resource: updated_resource) if change_set.append_id.present?
# Update after having appended a FileSet to a Work, or updated a Work
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract to method

app/decorators/file_set_decorator.rb Outdated
@@ -4,4 +4,12 @@ class FileSetDecorator < Valkyrie::ResourceDecorator
def manageable_files?
false
end

def parents
Valkyrie::MetadataAdapter.find(:indexing_persister).query_service.find_parents(resource: self).to_a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract query_service to its own method please.

spec/controllers/file_sets_controller_spec.rb
@@ -46,4 +52,14 @@
end
end
end

describe "DELETE /concern/file_sets/id" do
render_views
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary?

valhalla/app/controllers/concerns/valhalla/resource_controller.rb Outdated
@@ -23,6 +27,7 @@ def create
change_set_persister.buffer_into_index do |buffered_changeset_persister|
obj = buffered_changeset_persister.save(change_set: @change_set)
end
messenger.record_created(obj)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we need after_created in our change set persister.

@jrgriffiniii jrgriffiniii changed the title Adds support for RabbitMQ messaging [WIP] Adds support for RabbitMQ messaging Sep 20, 2017
@jrgriffiniii jrgriffiniii force-pushed the issues-104-jrgriffiniii-fire-rabbitmq-events branch from e3749c2 to 06e8826 Compare September 20, 2017 22:29
@jrgriffiniii jrgriffiniii force-pushed the issues-104-jrgriffiniii-fire-rabbitmq-events branch from e60aac3 to 94b6821 Compare September 25, 2017 15:00
@jrgriffiniii jrgriffiniii changed the title [WIP] Adds support for RabbitMQ messaging Adds support for RabbitMQ messaging Sep 25, 2017
@jrgriffiniii jrgriffiniii changed the title Adds support for RabbitMQ messaging [WIP] Adds support for RabbitMQ messaging Sep 25, 2017
@jrgriffiniii jrgriffiniii force-pushed the issues-104-jrgriffiniii-fire-rabbitmq-events branch 2 times, most recently from 5602b62 to 071a058 Compare September 26, 2017 16:44
@tpendragon
Copy link
Contributor

So I don't think those after_delete_commit messages are firing if you're actually in a transaction. This spec fails:

      it 'publishes messages in a transaction', run_real_derivatives: false, rabbit_stubbed: true do
        resource = FactoryGirl.build(:scanned_resource)
        change_set = change_set_class.new(resource, characterize: false)
        change_set.files = [file1]

        output = nil
        change_set_persister.buffer_into_index do |buffer|
          output = buffer.save(change_set: change_set)
        end

        expected_result = {
          "id" => output.id.to_s,
          "event" => "UPDATED",
          "manifest_url" => "http://www.example.com/concern/scanned_resources/#{output.id}/manifest",
          "collection_slugs" => []
        }

        expect(rabbit_connection).to have_received(:publish).at_least(:once).with(expected_result.to_json)
      end

Fixing this is hard though. I think we need to queue up events from the buffered persister, pass them into the un-buffered persister, and then run them.

@jrgriffiniii jrgriffiniii force-pushed the issues-104-jrgriffiniii-fire-rabbitmq-events branch from c29939b to c4fd76e Compare September 27, 2017 21:54
@jrgriffiniii
Resolves #104 by adding support for RabbitMQ messaging for the creati…
544f15d 
…on, updates, and deletion of Figgy resources

Removing the initializer for GeoBlacklight

Rebasing and updating in accordance with the refactored PlumChangeSetPersister

Remedying the broken test suites by more effectively stubbing messaging functionality

Refactoring the messaging broker integration for the PlumChangeSetPersister

Implementing test suites for the PublishMessage::Factory and MessagingClient

Restructuring PlumChangeSetPersister to use handlers for after_save_commit and after_delete_commit

Restoring the feature test for adding resources

Rebasing from e673e5e
@jrgriffiniii jrgriffiniii force-pushed the issues-104-jrgriffiniii-fire-rabbitmq-events branch from c4fd76e to 544f15d Compare September 28, 2017 13:21
@jrgriffiniii jrgriffiniii changed the title [WIP] Adds support for RabbitMQ messaging Adds support for RabbitMQ messaging Sep 28, 2017
@jrgriffiniii jrgriffiniii merged commit 2700ecd into master Sep 28, 2017
@jrgriffiniii jrgriffiniii deleted the issues-104-jrgriffiniii-fire-rabbitmq-events branch September 28, 2017 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tpendragon tpendragon tpendragon approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
09/25/17 - 09/29/17 Sprint
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jrgriffiniii @tpendragon