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

Fixes #210 and #208 #211

Merged
merged 6 commits into from Dec 23, 2021
Merged

Fixes #210 and #208 #211

merged 6 commits into from Dec 23, 2021

Conversation

guusdk
Copy link
Member

@guusdk guusdk commented Dec 14, 2021

Note that the fix for #210 requires OF-2352 / igniterealtime/Openfire#1945 which means that this PR will not work with 4.7.0 beta (but hopefully will with 4.7.0 non-beta).

@Fishbowler
Copy link
Member

Fishbowler commented Dec 20, 2021

Appears to "work" but seeing lots of instances of API misses on the plugin's pages. e.g.

GET http://xmpp1.localhost.example:9091/plugins/monitoring/api/buildprogress 404 (Not Found)

This doesn't occur on master, so I considered that it might be an effect of Jersey API wiring?

Tested using Openfire @ master (99e8b22)

@guusdk
Copy link
Member Author

guusdk commented Dec 20, 2021

This will only work with Openfire master after igniterealtime/Openfire#1945 has been merged (which currently isn't the case). Can you retry with this Openfire branch: guusdk:OF-2352_use-plugin-provided-servlet-context (or: review/merge that PR)?

@Fishbowler
Copy link
Member

Aw man, I didn't even check to see if it was merged 🤦
Will do.

@Fishbowler
Copy link
Member

Using a rebased version of this against a rebased version of OF-2352, the errors all disappear. Everything appears operation, except that I can't get Openfire to reliably recognise an active conversation. The users count goes up, but conversations regularly remains at 0. Suspect this might be related to which cluster node is senior.

Note that this change depends on OF-2352. This is _not_ in Openfire 4.7.0 beta (but should be merged before Openfire 4.7.0 is released).
@guusdk
Copy link
Member Author

guusdk commented Dec 21, 2021

This issue is caused by the new to-and-from XML serialization that was put in by #209 (to fix #120 and #170). The ConversationEvent class does not serialize at all. This fact is hidden by the implementation (you're not seeing exceptions), which is why this isn't very obvious. The net result is that a junior node tries to send stuff to the senior cluster node, where that data never arrives.

…onversationEvent

If ConversationEvent can't be marshalled to/from XML, then events won't ever be pushed successfully from junior cluster nodes to the senior node.
@guusdk
Copy link
Member Author

guusdk commented Dec 21, 2021

I've pushed a provisional change (I might want to clean this up a little), that should fix the problem. It seems to work for me.

…n-specific

The XmlSerializer was intended to be generic, but is used in only one invocation.

To make it easier to test the implementation, this commit turns the generic implementation into something that is specific to this plugin.
The DAO-like functionality that existed in the Conversation class made it hard to operate on instances. This commit splits off that functionality in a new, dedicated class named ConversationDAO.

No functional changes are expected from this.
Improved the XML-based serialization that got introduced as a fix for igniterealtime#120/igniterealtime#170

Dropped Externalization (supposed to be replaced with XML-based serialization) to reduce complexity.

Added unit tests to verify XML-based serialization functionality.
@guusdk
Copy link
Member Author

guusdk commented Dec 23, 2021

I've now pushed a more substantial fix, that also includes some unit tests that help verify the XML serialization bit. Should be good to go now.

@Fishbowler
Copy link
Member

My observed problem no longer occurs, and still fixes both intended issues 👍

@guusdk guusdk merged commit 4cfc7ba into igniterealtime:master Dec 23, 2021
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.

None yet

2 participants