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.

Sign up for GitHub

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

Jump to bottom

Resolve #2526: Index Scrubbing: eliminate missing record exception for dangl… #2527

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Resolve #2526: Index Scrubbing: eliminate missing record exception for dangl… #2527

wants to merge 3 commits into from
+77 −27

Conversation

jjezra
Copy link
Contributor

@jjezra jjezra commented Feb 22, 2024

…ing synthetic index entries.

@foundationdb-ci
Copy link
Contributor

Result of fdb-record-layer-pr on Linux CentOS 7

  • Commit ID: 4c2df49
  • Duration 0:37:42
  • Result: ❌ FAILED
  • Error: Error while executing command: ./gradlew --no-daemon --console=plain -b ./build.gradle build destructiveTest -PcoreNotStrict -PreleaseBuild=false -PpublishBuild=false -PspotbugsEnableHtmlReport. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@jjezra jjezra marked this pull request as draft February 23, 2024 13:06
@jjezra jjezra force-pushed the scrubber_avoid_asert_on_dangling_synthetic branch from 4c2df49 to 5708488 Compare February 23, 2024 14:24
@foundationdb-ci
Copy link
Contributor

Result of fdb-record-layer-pr on Linux CentOS 7

  • Commit ID: 5708488
  • Duration 0:48:06
  • Result: ❌ FAILED
  • Error: Error while executing command: ./gradlew --no-daemon --console=plain -b ./build.gradle build destructiveTest -PcoreNotStrict -PreleaseBuild=false -PpublishBuild=false -PspotbugsEnableHtmlReport. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@jjezra jjezra force-pushed the scrubber_avoid_asert_on_dangling_synthetic branch from 5708488 to 9a1e95e Compare February 23, 2024 18:16
@foundationdb-ci
Copy link
Contributor

Result of fdb-record-layer-pr on Linux CentOS 7

  • Commit ID: 9a1e95e
  • Duration 0:44:06
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@jjezra jjezra marked this pull request as ready for review February 23, 2024 21:01
@foundationdb-ci
Copy link
Contributor

Result of fdb-record-layer-pr on Linux CentOS 7

  • Commit ID: 1a6440b
  • Duration 0:47:02
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

alecgrieser
alecgrieser requested changes Feb 26, 2024
View reviewed changes
...src/main/java/com/apple/foundationdb/record/provider/foundationdb/IndexingScrubDangling.java Outdated
Comment on lines 196 to 199
final boolean isDangling = ((findException(ex, RecordDoesNotExistException.class) != null) ||
(syntheticRecord != null && syntheticRecord.getConstituents().isEmpty()));
if (isDangling) {
// None of the constituents of this synthetic type are present, so it must be dangling
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this will identify anything where any of the constituents is not found as dangling, which is different from the comment. It probably is the case that we want that behavior, I think.

It is a bit weird that this uses exception handling as control flow, though. If we wanted to fix that, I think we'd need a new variant of loadSyntheticRecord that takes an IndexOrphanBehavior argument, or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅

...c/test/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexScrubberTest.java
Assertions.assertTrue(0 < timer.getCount(FDBStoreTimer.Counts.ONLINE_INDEX_BUILDER_RECORDS_SCANNED));
assertEquals(0, timer.getCount(FDBStoreTimer.Counts.ONLINE_INDEX_BUILDER_RECORDS_INDEXED));
Assertions.assertTrue(0 < timer.getCount(FDBStoreTimer.Counts.INDEX_SCRUBBER_DANGLING_ENTRIES));
assertEquals(0, timer.getCount(FDBStoreTimer.Counts.INDEX_SCRUBBER_MISSING_ENTRIES));
Copy link
Contributor

Choose a reason for hiding this comment

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

It would probably be nice to extend this test so that we validate that it will delete the dangling entries if setAllowRepair is true and that that results in a clean index (similar to what the equivalent test for missing entries does).

...c/test/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexScrubberTest.java
indexScrubber.scrubMissingIndexEntries();
}
Assertions.assertTrue(0 < timer.getCount(FDBStoreTimer.Counts.ONLINE_INDEX_BUILDER_RECORDS_SCANNED));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can be more precise with this assertion. At the very least, this should be:

assertThat(timer.getCount(FDBStoreTimer.Counts.ONLINE_INDEX_BUILDER_RECORDS_SCANNED), greaterThan(0));

To make the assertion a little more readable, but we actually know the expected number of records to scan should be 5 * numRecords, I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅

...c/test/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexScrubberTest.java
Assertions.assertTrue(0 < timer.getCount(FDBStoreTimer.Counts.ONLINE_INDEX_BUILDER_RECORDS_SCANNED));
assertEquals(0, timer.getCount(FDBStoreTimer.Counts.ONLINE_INDEX_BUILDER_RECORDS_INDEXED));
Assertions.assertTrue(0 < timer.getCount(FDBStoreTimer.Counts.INDEX_SCRUBBER_DANGLING_ENTRIES));
assertEquals(0, timer.getCount(FDBStoreTimer.Counts.INDEX_SCRUBBER_MISSING_ENTRIES));
Copy link
Contributor

Choose a reason for hiding this comment

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

In theory, we could do with an additional test here that validates the behavior if there's a join index and one (but not all) of the constituents is deleted. I believe, as written, it will identify those as dangling (and I think that's right)

@jjezra jjezra force-pushed the scrubber_avoid_asert_on_dangling_synthetic branch from 1a6440b to 6b95749 Compare April 8, 2024 15:17
@foundationdb-ci
Copy link
Contributor

Result of fdb-record-layer-pr on Linux CentOS 7

  • Commit ID: 6b95749
  • Duration 0:39:09
  • Result: ❌ FAILED
  • Error: Error while executing command: ./gradlew --no-daemon --console=plain -b ./build.gradle build destructiveTest -PcoreNotStrict -PreleaseBuild=false -PpublishBuild=false -PspotbugsEnableHtmlReport. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@jjezra jjezra marked this pull request as draft August 13, 2024 19:20
@jjezra jjezra changed the title Resolve #2526: Scrubber: eliminate missing record exception for dangl… Resolve #2526: Index Scrubbing: eliminate missing record exception for dangl… Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alecgrieser alecgrieser alecgrieser requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jjezra @foundationdb-ci @alecgrieser