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

[PDS-158510] Improved Detection of Orphaned Sentinels #5231

Merged
merged 8 commits into from
Feb 2, 2021

Conversation

jeremyk-91
Copy link
Contributor

Goals (and why):

  • PDS-158510: some users were running into issues with orphaned sentinel skip not working.

Implementation Description (bullets):

  • In safe targeted sweep table destruction #3610 detection and skipping of sweep sentinels was added. However, there is an edge case: if a transaction T1 reads an orphaned sentinel S, for which an uncommitted T2 has written to the KVS, then T1 doesn't recognise S as orphaned even though the value T2 covered it with is uncommitted.

Testing (What was existing testing like? What have you done to improve it?):
Added a new group of tests.

Concerns (what feedback would you like?):
Not much. Is perf a concern?

Where should we start reviewing?: Probably start with the tests

Priority (whenever / two weeks / yesterday): yesterday

@jeremyk-91 jeremyk-91 requested a review from gmaretic February 2, 2021 13:22
@changelog-app
Copy link

changelog-app bot commented Feb 2, 2021

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Orphaned sentinels are now correctly skipped over. Previously, an orphaned sentinel covered by a value written by a (possibly uncommitted) transaction would not be considered orphaned when read until Sweep or another read transaction cleaned up the data written by said uncommitted transaction.

Check the box to generate changelog(s)

  • Generate changelog entry

Copy link
Contributor

@gmaretic gmaretic left a comment

Choose a reason for hiding this comment

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

Paired on most of this.

A few nits. I think perf is fine, took a look at the metrics for encountering naked sentinels and it's not very common. The reads are going to dominate the perf of this either way and there is no way around that.

}

@Test
public void getSweepSentinelUnderEarlyUncommittedWriteDoesNotThrow() {
Copy link
Contributor

Choose a reason for hiding this comment

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

should swap the names of this and the following test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, you're right!


@SuppressWarnings("unchecked") // ArgumentCaptor
@Test
public void getSentinelValuesStressTest() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice 👍

.collect(Collectors.toList());
assertThat(stressTestRequests.get(index)).hasSizeLessThan(previousTimestamps.size());
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

assert that retry succeeds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, makes sense - added.

@bulldozer-bot bulldozer-bot bot merged commit 4b9ae19 into develop Feb 2, 2021
@bulldozer-bot bulldozer-bot bot deleted the jkong/sentinels-eveywhere branch February 2, 2021 15:17
@svc-autorelease
Copy link
Collaborator

Released 0.290.2

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