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

Fix #1898: Indexing: periodic "Built Range" log message is broken #1899

Merged
merged 2 commits into from
Nov 28, 2022
Merged

Fix #1898: Indexing: periodic "Built Range" log message is broken #1899

merged 2 commits into from
Nov 28, 2022

Conversation

jjezra
Copy link
Contributor

@jjezra jjezra commented Nov 7, 2022

No description provided.

jleach4
jleach4 suggested changes Nov 7, 2022
View reviewed changes
Copy link
Contributor

@jleach4 jleach4 left a comment

Choose a reason for hiding this comment

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

Test?

@foundationdb-ci
Copy link
Contributor

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

@foundationdb-ci
Copy link
Contributor

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

@jjezra
Copy link
Contributor Author

jjezra commented Nov 7, 2022

Test?

I've verified that the log message appears in some of the existing tests. Example:

2022-11-07 15:54:55,826 [INFO] c.a.f.r.p.f.IndexingBase - Indexer: Built Range calling_method="buildMultiTargetIndex" delay="1" indexer_id="c016639b-30cc-47d4-98bd-5810e2bd4186" indexing_method="multi target by records" limit="17" range_end="null" range_start="null" records_per_second="10000" records_scanned="17" subspace="\x15\x1a" target_index_name="[indexA, indexB, indexC, indexD]"

ScottDugas
ScottDugas reviewed Nov 7, 2022
View reviewed changes
...yer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/IndexingBase.java Outdated
@@ -622,7 +624,7 @@ private void validateTimeLimit(int toWait) {
private boolean shouldLogBuildProgress() {
long interval = common.config.getProgressLogIntervalMillis();
long now = System.currentTimeMillis();
if (interval == 0 || interval < (now - timeOfLastProgressLogMillis)) {
if (interval == 0 || interval > (now - timeOfLastProgressLogMillis)) {
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 you could combine a sleep in the IndexingConfig loader, and TestHelpers.assertLogs to assert that this is actually logging, and at approximately the right interval.
I don't think the record layer already has anything for mocking out time, but that would probably be beneficial for other tests if you brought something in.

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 can do that, but TestHelpers.assertLogs will not test the interval but just the existence of this message (appearing at first continuation - i.e. in any case of not indexing all in a single transaction).

@foundationdb-ci
Copy link
Contributor

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

  • Commit ID: e26a351
  • Duration 0:07:41
  • Result: ❌ FAILED
  • Error: Error while executing command: ./gradlew --no-daemon --console=plain -b ./build.gradle build destructiveTest sonarqube -PcoreNotStrict -PreleaseBuild=false -PpublishBuild=false -PspotbugsEnableHtmlReport. Reason: exit status 1
  • Build Logs (available for 30 days)
  • Build Artifact (available for 30 days)

@foundationdb-ci
Copy link
Contributor

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

@foundationdb-ci
Copy link
Contributor

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

@foundationdb-ci
Copy link
Contributor

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

@jjezra
Copy link
Contributor Author

jjezra commented Nov 7, 2022

FDBRecordStoreIndexTest > testBoundaryPrimaryKeys() FAILED

Test this please

@jjezra
Copy link
Contributor Author

jjezra commented Nov 7, 2022

Squashed.

@foundationdb-ci
Copy link
Contributor

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

@foundationdb-ci
Copy link
Contributor

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

@jjezra jjezra requested a review from jleach4 November 7, 2022 22:55
alecgrieser
alecgrieser requested changes Nov 8, 2022
View reviewed changes
...yer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/IndexingBase.java Outdated
@@ -622,7 +624,7 @@ private void validateTimeLimit(int toWait) {
private boolean shouldLogBuildProgress() {
long interval = common.config.getProgressLogIntervalMillis();
long now = System.currentTimeMillis();
if (interval == 0 || interval < (now - timeOfLastProgressLogMillis)) {
if (interval == 0 || interval > (now - timeOfLastProgressLogMillis)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm reading the Javadoc for getProgressLogIntervalMillis correctly, I think this is still not quite right:

Get the minimum time between successful progress logs when building across transactions. Negative will not log at all, 0 will log after every commit.

So, if interval == 0, then I think this method should always return true, whereas right now, it always returns false. Likewise, if interval < 0, then I think this method should always return false, but I believe it will always return true as now - timeOfLastProgressLogMillis will always be non-negative.

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/OnlineIndexerSimpleTest.java
openSimpleMetaData(hook);
try (OnlineIndexer indexer = OnlineIndexer.newBuilder()
.setDatabase(fdb).setMetaData(metaData).setIndex(index).setSubspace(subspace)
.setProgressLogIntervalMillis(10)
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 adding test cases where the progressLogIntervalMillis is 0 (and logs) and -1 (and doesn't log) may be worth it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅

@foundationdb-ci
Copy link
Contributor

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

  • Commit ID: 5836116
  • Duration 0:01:45
  • Result: ❌ FAILED
  • Error: Error while executing command: ./gradlew --no-daemon --console=plain -b ./build.gradle build destructiveTest sonarqube -PcoreNotStrict -PreleaseBuild=false -PpublishBuild=false -PspotbugsEnableHtmlReport. Reason: exit status 1
  • Build Logs (available for 30 days)
  • Build Artifact (available for 30 days)

@foundationdb-ci
Copy link
Contributor

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

  • Commit ID: 5836116
  • Duration 0:01:43
  • Result: ❌ FAILED
  • Error: Error while executing command: ./gradlew --no-daemon --console=plain -b ./build.gradle build destructiveTest sonarqube -PcoreNotStrict -PreleaseBuild=false -PpublishBuild=false -PspotbugsEnableHtmlReport. Reason: exit status 1
  • Build Logs (available for 30 days)
  • Build Artifact (available for 30 days)

@jjezra
Fix FoundationDB#1898: Indexing: periodic "Built Range" log message i…
505855c 
…s broken

    squashed:
    + Implement John's and Scott's requested change - adding a test
@jjezra
Apply Alec's requested changes
1bccf9c
@sonarcloud
Copy link

sonarcloud bot commented Nov 8, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@foundationdb-ci
Copy link
Contributor

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

@foundationdb-ci
Copy link
Contributor

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

@jjezra jjezra requested review from jleach4 and removed request for jleach4 November 12, 2022 08:29
@jjezra jjezra merged commit 238a4f2 into FoundationDB:main Nov 28, 2022
@jjezra jjezra deleted the io_fix_built_range_log_msg branch November 28, 2022 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ScottDugas ScottDugas ScottDugas left review comments

@jleach4 jleach4 jleach4 approved these changes

@alecgrieser alecgrieser alecgrieser 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.

None yet
5 participants
@jjezra @foundationdb-ci @ScottDugas @jleach4 @alecgrieser