OF-2437: Improve CachingPubsubPersistenceProvider #2664
Conversation
|
@Fishbowler do you think that the test approach is valid? Also, is the behavior (as tested, and documented in the test) consistent and correct? |
guusdk
force-pushed
the
OF-2437_CachingPubSubPersistenceProvider_fixes
branch
6 times, most recently
from
565c5f1 to
81c56e8
Compare
These new unit tests verify behavior of CachingPubsubPersistenceProvider (which fails, as indicated by some of these test failing). The tests introduced here cover four types of operations that are being scheduled by the implementation: - Node creation/changing/removal - Subscription creation/update/removal - Affiliation creation/update/removal - publishing/removal of items
guusdk
force-pushed
the
OF-2437_CachingPubSubPersistenceProvider_fixes
branch
from
81c56e8 to
0a50b8b
Compare
The changes in this commit address the issues that are exposed in the unit tests that are added in the previous commit.
guusdk
changed the title
|
The second commit in this PR fixes the issues that are exposed by the unit tests that are added in the first commit. |
|
The added unit tests do not verify assert the result of obtaining data from a provider. This is certainly something I'd like to (eventually) add, but this might lead to a refactoring of the interface (the current interface definition requires a caller to pass along an instance of the pubsub service on which the data is to be loaded - that's awkward design, making it hard to write tests for). |
|
Note that I'm not sure if any of these fixes actually address OF-2437. Given that this issue appears to be specific to the caching provider, I think that there's a good chance. Sadly, we do not know how to reproduce the issue. I think that leaves us with dogfooding, to see if the problem persists after these changes. |
...ava/org/jivesoftware/openfire/pubsub/CachingPubsubPersistenceProviderItemOperationsTest.java
Outdated
Show resolved
Hide resolved
...ava/org/jivesoftware/openfire/pubsub/CachingPubsubPersistenceProviderItemOperationsTest.java
Outdated
Show resolved
Hide resolved
...ava/org/jivesoftware/openfire/pubsub/CachingPubsubPersistenceProviderItemOperationsTest.java
Outdated
Show resolved
Hide resolved
.../jivesoftware/openfire/pubsub/CachingPubsubPersistenceProviderAffiliationOperationsTest.java
Outdated
Show resolved
Hide resolved
.../jivesoftware/openfire/pubsub/CachingPubsubPersistenceProviderAffiliationOperationsTest.java
Outdated
Show resolved
Hide resolved
...ava/org/jivesoftware/openfire/pubsub/CachingPubsubPersistenceProviderNodeOperationsTest.java
Outdated
Show resolved
Hide resolved
...ava/org/jivesoftware/openfire/pubsub/CachingPubsubPersistenceProviderNodeOperationsTest.java
Show resolved
Hide resolved
...jivesoftware/openfire/pubsub/CachingPubsubPersistenceProviderSubscriptionOperationsTest.java
Show resolved
Hide resolved
…ingPubsubPersistenceProviderItemOperationsTest.java Co-authored-by: Dan Caseley <dan@caseley.me.uk>
…ingPubsubPersistenceProviderAffiliationOperationsTest.java Co-authored-by: Dan Caseley <dan@caseley.me.uk>
…ingPubsubPersistenceProviderItemOperationsTest.java Co-authored-by: Dan Caseley <dan@caseley.me.uk>
…ingPubsubPersistenceProviderNodeOperationsTest.java Co-authored-by: Dan Caseley <dan@caseley.me.uk>
…ingPubsubPersistenceProviderItemOperationsTest.java Co-authored-by: Dan Caseley <dan@caseley.me.uk>
…ingPubsubPersistenceProviderAffiliationOperationsTest.java Co-authored-by: Dan Caseley <dan@caseley.me.uk>
There was a problem hiding this comment.
Discussed queue optimisations, which we've decided could only go wrong by another bug elsewhere in the business logic or database layer. Adding defensive code, or a flag to reduce optimisation, would only serve to hide (or exacerbate) that problem, which feels like smell.
This new unit test verifies behavior of CachingPubsubPersistenceProvider (which fails, as indicated by some of these test failing).
The tests introduced here only cover one (of three) types of operations that are being scheduled by the implementation:
These two other types of operation still need to be added: