Skip to content
This repository has been archived by the owner on Nov 14, 2024. It is now read-only.

Remove LockWatchStateUpdate#Failed. #4820

Merged
merged 11 commits into from
Jun 25, 2020
Merged

Conversation

jkozlowski
Copy link
Contributor

@jkozlowski jkozlowski commented Jun 2, 2020

Goals (and why):

Try to simplify the code by removing a response that we should be able to not have.

Implementation Description (bullets):

If the client is too far behind, calculate a snapshot. Operations that touch the log, always lock it exclusively (whether for reading or writing), therefore we can always calculate a cohesive snapshot: all mutations will wait while we are calculating the snapshot, therefore the client will then later be able to get all the events. They might get the same "event" multiple times (because HeldLocks iteration is not blocking), but the ordering of these events will always be causal (e.g. unlock always comes after lock), but they should be able to deal with that

Testing (What was existing testing like? What have you done to improve it?):

Existing unit tests

Concerns (what feedback would you like?):

Whether there is any problems with this approach.

Where should we start reviewing?:

Priority (whenever / two weeks / yesterday):

@jkozlowski jkozlowski changed the title [WIP] Failure is not an option 2 [WIP] Remove LockWatchStateUpdate#Failed. Jun 2, 2020
@jkozlowski jkozlowski changed the title [WIP] Remove LockWatchStateUpdate#Failed. Remove LockWatchStateUpdate#Failed. Jun 23, 2020
@jkozlowski jkozlowski requested a review from jeremyk-91 June 23, 2020 21:55
@jkozlowski
Copy link
Contributor Author

jkozlowski commented Jun 24, 2020

To be fair, I think I might do another PR where we'll actually keep the still open locks in the log, instead of a fixed size. Then snapshot calculation is just a matter of copying the whole log. They are already in memory, so shouldn't be a big deal.

Actually, to account for slow consumers we'd still need to keep some backlog likely, but at least the snapshot calculation would be easier and less weird.

Copy link
Contributor

@jeremyk-91 jeremyk-91 left a comment

Choose a reason for hiding this comment

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

⏲️ 0:20

Broadly looks good! Minor questions on some of the edge-cases of semantics on the array window.

I can also imagine a version of this with read write locks (upside: would allow decent concurrency if, as intended, watched elements are hardly ever written to) though let's stick with synchronized for now. Having all accesses synchronized makes convincing myself that our usage is safe pretty straightforward.

The tracking of the open locks in memory seems reasonable (as before, if we're watching things that aren't meant to be frequently updated...)

Copy link
Contributor

@jeremyk-91 jeremyk-91 left a comment

Choose a reason for hiding this comment

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

👍 looks good!

@bulldozer-bot bulldozer-bot bot merged commit 27dbba7 into develop Jun 25, 2020
@bulldozer-bot bulldozer-bot bot deleted the failure-is-not-an-option-2 branch June 25, 2020 10:52
@svc-autorelease
Copy link
Collaborator

Released 0.234.0

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants