Present Publisher as offline to Subscribers on loss of connectivity on the Subscriber's side #855
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…me unnecessary indirection. There are still improvements I could make, but I think this makes the code easier to follow, especially now that we have a single emitEventsIfRequired() method. I ended up removing all of the worker queue tests as it was unclear to me what value they were adding to the codebase. They were hampering refactoring speed, due to my change to the SubscriberProperties constructor signature.
… uniquely identifying the source of presence messages.
…er instances. So, for example, this fixes behaviour for the sequence: - Publisher with Member Key A:1 Enters Presence - Publisher with Member Key A:2 Enters Presence - Publisher with Member Key A:1 Leaves Presence Previously this would flag "the" publisher as offline. With this change, it will now continue to flag "the" publisher as online.
…e our local connectivity state being offline.
This was referenced Dec 19, 2022
ikbalkaya
previously requested changes
Dec 20, 2022
There was a problem hiding this comment.
As this PR contains mostly refactoring code, I wasn't able to confirm where exactly the change happened
I assume that's isPublisherVisible but wasn't sure - could you please provide an explanation to PR description so this is more clear?
Also about tests, as far as I can see they do test the individual workers, I think they are still important as they protect the correct functioning of individual workers.
subscribing-sdk/src/main/java/com/ably/tracking/subscriber/CoreSubscriber.kt
Show resolved
Hide resolved
subscribing-sdk/src/main/java/com/ably/tracking/subscriber/CoreSubscriber.kt
Show resolved
Hide resolved
subscribing-sdk/src/main/java/com/ably/tracking/subscriber/CoreSubscriber.kt
Outdated
Show resolved
Hide resolved
...src/test/java/com/ably/tracking/subscriber/workerqueue/workers/ChangeResolutionWorkerTest.kt
Outdated
Show resolved
Hide resolved
...g-sdk/src/test/java/com/ably/tracking/subscriber/workerqueue/workers/DisconnectWorkerTest.kt
Outdated
Show resolved
Hide resolved
davyskiba
suggested changes
Dec 20, 2022
...a/com/ably/tracking/subscriber/workerqueue/workers/SubscribeForPresenceMessagesWorkerTest.kt
Outdated
Show resolved
Hide resolved
.../src/test/java/com/ably/tracking/subscriber/workerqueue/workers/StartConnectionWorkerTest.kt
Outdated
Show resolved
Hide resolved
...-sdk/src/main/java/com/ably/tracking/subscriber/workerqueue/workers/StartConnectionWorker.kt
Show resolved
Hide resolved
subscribing-sdk/src/main/java/com/ably/tracking/subscriber/CoreSubscriber.kt
Show resolved
Hide resolved
subscribing-sdk/src/main/java/com/ably/tracking/subscriber/CoreSubscriber.kt
Outdated
Show resolved
Hide resolved
subscribing-sdk/src/main/java/com/ably/tracking/subscriber/CoreSubscriber.kt
Outdated
Show resolved
Hide resolved
subscribing-sdk/src/main/java/com/ably/tracking/subscriber/CoreSubscriber.kt
Outdated
Show resolved
Hide resolved
|
Recognising the push back from both @ikbalkaya and @davyskiba on my removal of tests, I'm going to move this PR back to Draft state while I look at those again. |
I initially removed them in 130dfa6 but, quite rightly, had push back from other members of the team so I have put time into reinstating them which required refactors to reflect my changes elsewhere.
QuintinWillison
requested review from
ikbalkaya,
davyskiba and
lawrence-forooghian
davyskiba
approved these changes
Dec 22, 2022
QuintinWillison
dismissed
ikbalkaya’s stale review
Ikbal is on holiday and we need to get this pull request merged.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
More of a refactor than perhaps entirely necessary but I was finding the current subscriber state emission code difficult to follow and reason about.
In the same vein, I've also removed a number of tests which were (in my opinion) adding little value to the maintainability of the codebase as we go forwards - I'm sure they were helpful when creating the worker queue infrastructure but at this point they were a major barrier to refactoring.UPDATE: Tests reinstated in fca0789
Fixes #833.