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

Add a test that does concurrent lucene updates #2996

Merged
merged 3 commits into from
Dec 12, 2024
Merged

Add a test that does concurrent lucene updates #2996

merged 3 commits into from
Dec 12, 2024

Conversation

ScottDugas
Copy link
Contributor

@ScottDugas ScottDugas commented Dec 5, 2024 •
edited
Loading

This adds a new test that does concurrent updates to different records in lucene.
The test references two specific issues:

And configures itself so as to avoid hitting those issues, but as those are fixed, we can expand the test to include coverage for those issues.

@ScottDugas ScottDugas force-pushed the concurrent-lucene-updates branch 2 times, most recently from 2891137 to 2fe1200 Compare December 5, 2024 21:59
@foundationdb-ci
Copy link
Contributor

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

  • Commit ID: c7c3143
  • Duration 0:46:34
  • 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)

@foundationdb-ci
Copy link
Contributor

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

  • Commit ID: 2891137
  • Duration 0:46:02
  • 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)

@foundationdb-ci
Copy link
Contributor

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

  • Commit ID: 2fe1200
  • Duration 0:46:02
  • 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)

@ScottDugas ScottDugas force-pushed the concurrent-lucene-updates branch from 2fe1200 to 5f3f6bf Compare December 6, 2024 15:25
@foundationdb-ci
Copy link
Contributor

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

  • Commit ID: 5f3f6bf
  • Duration 0:46:35
  • 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)

@ScottDugas ScottDugas force-pushed the concurrent-lucene-updates branch from 5f3f6bf to c694659 Compare December 6, 2024 20:46
@foundationdb-ci
Copy link
Contributor

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

  • Commit ID: c694659
  • Duration 0:48:11
  • 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)

ScottDugas
ScottDugas commented Dec 9, 2024
View reviewed changes
...ayer-lucene/src/test/java/com/apple/foundationdb/record/lucene/LuceneIndexTestValidator.java
Set<Integer> usedPartitionIds = new HashSet<>();
Tuple lastToTuple = null;
int visitedCount = 0;
if (partitionHighWatermark > 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tip: This code is much easier to review if you hide whitespace changes, because the bulk of validateUnpartitionedGroup is the same, just indented.

@ScottDugas ScottDugas marked this pull request as ready for review December 9, 2024 16:03
ohadzeliger
ohadzeliger requested changes Dec 10, 2024
View reviewed changes
...er-lucene/src/test/java/com/apple/foundationdb/record/lucene/LuceneIndexMaintenanceTest.java Show resolved Hide resolved
...er-lucene/src/test/java/com/apple/foundationdb/record/lucene/LuceneIndexMaintenanceTest.java Show resolved Hide resolved
...ayer-lucene/src/test/java/com/apple/foundationdb/record/lucene/LuceneIndexTestDataModel.java
@@ -64,6 +66,7 @@ public class LuceneIndexTestDataModel {
* </p>
*/
final Map<Tuple, Map<Tuple, Tuple>> groupingKeyToPrimaryKeyToPartitionKey;
Map<Tuple, Function<Message, Message>> updateableRecords;
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems very specific and limited - won't it make sense to store the records as "saved records" and handle the update-specific logic in the test class rather than at the model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I may be hyper-focused on the maintenance tests which have less interesting record shapes, but I feel like, if we want to pull this out, I would also want to abstract away the types a little bit, so that the code above doesn't have to worry about whether it is synthetic or not.
Or perhaps, that the Function here should be replaced with an object, that has a variety of operations.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the sake of making progress, then, would you like to check this in as-is and postpone these improvements to a later pr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this to be an object in #3003 but I think there is definitely room to grow, and polish there.

...er-lucene/src/test/java/com/apple/foundationdb/record/lucene/LuceneIndexMaintenanceTest.java Show resolved Hide resolved
@foundationdb-ci
Copy link
Contributor

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

  • Commit ID: e666678
  • Duration 0:47:47
  • 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)

@ScottDugas
Merge branch 'main' into concurrent-lucene-updates
bc88d46 
There were conflicts around LuceneIndexTestDataModel, particularly the `saveRecord` methods
because main added withContent, and this branch added updateableRecords
@foundationdb-ci
Copy link
Contributor

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

  • Commit ID: bc88d46
  • Duration 0:49:41
  • 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)

@ScottDugas ScottDugas merged commit 93d4b97 into FoundationDB:main Dec 12, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ohadzeliger ohadzeliger ohadzeliger approved these changes

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
@ScottDugas @foundationdb-ci @ohadzeliger