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

STOMP message persistence #840

Merged
merged 1 commit into from
Sep 30, 2021

Conversation

adam-vessey
Copy link

@adam-vessey adam-vessey commented Jun 23, 2021

JIRA Ticket: N/A

  • Other Relevant Links (Google Groups discussion, related pull requests, Release pull requests, etc.)

Contains #839, slightly reworking and adding a fix for Islandora/documentation#1857

What does this Pull Request do?

Adds the header such that STOMP messages should be persistent.

What's new?

Renamed the event handler method introduced in #839 to be less specific to auth, to add in the persistent header to be true, if it's not already set, allowing for the the case somebody ever desires things be persistent, such that they could implement a listener/subscriber and explicitly set it as false.

How should this be tested?

Somewhat predicated on having ActiveMQ itself configured for persistence:

  1. disable Karaf
  2. trigger a derivative processed via Crayfish which is expected to succeed
  3. restart Active MQ
  4. enable Karaf

Without this PR, the message would be lost when restarting ActiveMQ.
With this PR, the message should be persisted to disk when ActiveMQ stops and loaded when it starts, and Karaf should receive and process the message accordingly.

The message persistence might also be verified via ActiveMQ's web ui.

Additional Notes:

  • Does this change the interface, add a new feature, or otherwise change behaviours that would require updating documentation? Don't believe so.
  • Does this change add any new dependencies? No.
  • Does this change require any other modifications to be made to the repository (ie. Regeneration activity, etc.)? No.
  • Could this change impact execution of existing code? Unlikely.

Interested parties

@Islandora/8-x-committers

@adam-vessey
Copy link
Author

Rebased onto #839

@whikloj
Copy link
Member

whikloj commented Sep 29, 2021

Hey @adam-vessey is there some configuration I need to do.

Using a playbook installation and #839, I wanted to verify the failure first.

I take Karaf down, create a new Repository Item and add a Media to it (I used a Tiff).

The Repository Item doesn't get persisted to Fedora and none of the derivatives generated.

But when I bring Karaf back up they do start getting processed without the additional commit of this PR. Wondering if there is some setup I need.

I had assumed that if ActiveMQ is running and you've defined a queue (and not a topic) then the message would wait around for a consumer. Which is what I am seeing.

@adam-vessey
Copy link
Author

@whikloj: The messages would still be kept in memory by ActiveMQ, if it's not restarted... from what you've described, you've only done steps 1, 2, and 4; missing part 3 (which is where the persistence itself is actually tested).

@whikloj
Copy link
Member

whikloj commented Sep 29, 2021

DOH 🤦

@whikloj
Copy link
Member

whikloj commented Sep 29, 2021

Ok @adam-vessey I've tested this and it works. But I also merged #839 to have it go as its own commit. So when you get a chance to rebase this PR on main again I'll merge it. Thanks for all your work 🥇

@adam-vessey adam-vessey force-pushed the upstream-stomp-persistence branch from 3bc7113 to 76aad0a Compare September 29, 2021 20:08
@adam-vessey
Copy link
Author

Rebased down to the one differing commit.

Copy link
Member

@whikloj whikloj left a comment

Choose a reason for hiding this comment

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

👍

@whikloj whikloj merged commit 05c0d1c into Islandora:8.x-1.x Sep 30, 2021
@adam-vessey adam-vessey deleted the upstream-stomp-persistence branch July 18, 2022 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants