Use exact numDocs in synced-flush and metadata snapshot #30228

dnhatn · 2018-04-28T03:00:33Z

Since #29458, we use a searcher to calculate the number of documents for a commit stats. Sadly, that approach is flawed. The searcher might no longer point to the last commit if it's refreshed. As synced-flush requires an exact numDocs to work correctly, we have to exclude all soft-deleted docs.

This commit makes synced-flush stop using CommitStats but read an exact numDocs directly from an index commit.

Relates #29458
Relates #29530

Since elastic#29458, we use a searcher to calculate the number of documents for a commit stats. Sadly, that approach is flawed. The searcher might no longer point to the last commit if it's refreshed. This commit uses SoftDeletesDirectoryReaderWrapper to exclude the soft-deleted documents from numDocs in a SegmentInfos. I chose to modify the method Luence#getNumDocs so that we can read a store metadata snapshot correctly without opening an engine. Relates elastic#29458

elasticmachine · 2018-04-28T03:00:35Z

Pinging @elastic/es-distributed

It might be incorrect if forceMerge happens

dnhatn · 2018-04-29T15:14:35Z

@elasticmachine test this please

bleskes · 2018-04-30T08:09:13Z

I wonder if we should do this. Classically number of docs was always total docs in the segments. I'm not saying this isn't debatable but I'm also not sure it merits the breakage price. Also we want this to go into 6.x. Instead of this, maybe we should another field to the stats to reflect soft deletes? then sycned flush is freed to do what it thinks is right and we have more information.

s1monw

left one comment

s1monw · 2018-04-30T08:23:09Z

server/src/main/java/org/elasticsearch/common/lucene/Lucene.java

     */
-    public static int getNumDocs(SegmentInfos info) {


I think we should keep this like it is. We should rather, in the engine do this conditionally if we have soft-deletes enabled. Also please make sure that we only load this once and cache it. We don't commit very often and commit stats are fetched rarely we can do this when it's needed and cache it per commit.

dnhatn · 2018-04-30T15:19:43Z

@bleskes and @simonw

I understand your concerns. However, we get numDocs from a commit not only when sealing an index but also when verifying sync_id in peer recovery. In peer recovery, we read numDocs directly from Store as a target shard has not opened its engine yet. Thus, fixing this in an engine might not be enough.

elasticsearch/server/src/main/java/org/elasticsearch/indices/recovery/RecoverySourceHandler.java

Line 319 in 45c6c20

if (numDocsTarget != numDocsSource) {

elasticsearch/server/src/main/java/org/elasticsearch/index/store/Store.java

Line 865 in 45c6c20

numDocs = Lucene.getNumDocs(segmentCommitInfos);

Since we don't seal an index if numDocs are different, is it ok to remove the numDocs checking at recovery time?

bleskes · 2018-04-30T15:23:12Z

@dnhatn Simon will be a better judge, but I think we need a better utility method like Lucene. getNumDocs that takes soft deletes into account? we can work directly on the index for this?

dnhatn · 2018-04-30T15:45:05Z

I think we need a better utility method like Lucene. getNumDocs that takes soft deletes into account?

@bleskes Yes, we should do it.

dnhatn · 2018-05-01T02:26:02Z

@s1monw I take a different approach for this. Can you please have a look? Thank you.

dnhatn · 2018-05-01T05:19:51Z

@elasticmachine retest this please

This is tracked by #30228

s1monw · 2018-05-03T06:31:08Z

I am ok with the changing the semantics here. I think we try to make something work that isn't necessarily giving us enough information. I think we should keep it the way it is and deprecate it in 6.x? I really wonder what the purpose of this stats is compared to other stats that have a live numDocs that is accurate?

s1monw · 2018-05-03T11:29:14Z

Double checking - the way it is in master or in the PR? can you clarify?

the way it is in master

other stats that have a live numDocs that is accurate?

I mean for instance docStats here: https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-stats.html

# Conflicts: # server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

This is tracked by #30228

dnhatn · 2018-05-10T02:02:23Z

@s1monw and @bleskes,

I've updated the PR to restore the CommitStats and made synced-flush using an exact numDocs only when soft-deletes is enabled. There are still some issues that I am not sure.

With this change, only the synced-flush call uses an exact numDocs. Should we still need to cache this value in an engine?
In peer-recovery, a replica shard needs to send an exact numDocs to the primary shard if soft-deletes is enabled and an index was sealed. I see two possible approaches:

Pass an extra parameter "requiredExactNumDocs" in getStoreMetadataSnapshot. Internally we always calculate an exact numDocs if this parameter is true. This does not optimize the case when the index was not sealed and may cause confusion for other StoreMetadataSnapshot's usages.
Override the numDocs by an exact numDocs when the soft-deletes is true and an index was sealed. This is not nice but the change is minimal and does not affect StoreMetadataSnapshot's other usages.

Please have a look and let me know your thought. Thank you!

bleskes · 2018-05-10T09:27:50Z

server/src/main/java/org/elasticsearch/indices/recovery/PeerRecoveryTargetService.java

+            // The primary shard may need the "exact" numDocs to verify if the commit has syncId.
+            final boolean softDeleteEnabled = recoveryTarget.indexShard().indexSettings().isSoftDeleteEnabled();
+            if (softDeleteEnabled && Strings.hasText(snapshot.getSyncId())) {
+                final List<IndexCommit> commits = DirectoryReader.listCommits(recoveryTarget.store().directory());


This needs to use safe commit. This is the commit the engine will use (probably an existing bug) .

bleskes · 2018-05-10T09:30:11Z

Thx @dnhatn . I like where this is going. IMO neither synced flush nor recover are frequent operations. We should just use exact numbers all the time and everywhere. keep it simple. if soft deletes are not enabled, it's the same as the "fast" numDocs (and we can assert for that).

s1monw

LGTM

s1monw · 2018-05-14T14:01:46Z

server/src/main/java/org/elasticsearch/common/lucene/Lucene.java

+     * Unlike {@link #getNumDocs(SegmentInfos)} this method returns a numDocs that always excludes soft-deleted docs.
+     * This method is expensive thus prefer using {@link #getNumDocs(SegmentInfos)} unless an exact numDocs is required.
+     */
+    public static int getExactNumDocs(IndexCommit commit) throws IOException {


s1monw · 2018-05-14T14:03:11Z

server/src/main/java/org/elasticsearch/indices/flush/SyncedFlushService.java

+            final SegmentInfos segmentInfos = Lucene.readSegmentInfos(commitRef.getIndexCommit());
+            final int numDocs;
+            if (indexShard.indexSettings().isSoftDeleteEnabled()) {
+                numDocs = Lucene.getExactNumDocs(commitRef.getIndexCommit());


you can call the getExactNumDocs all the time. it has no overhead if soft_deletes are not there

s1monw · 2018-05-14T14:04:30Z

server/src/main/java/org/elasticsearch/indices/recovery/RecoverySourceHandler.java

@@ -341,7 +342,8 @@ public void phase1(final IndexCommit snapshot, final Supplier<Integer> translogO
                    recoverySourceSyncId.equals(recoveryTargetSyncId);
            if (recoverWithSyncId) {
                final long numDocsTarget = request.metadataSnapshot().getNumDocs();
-                final long numDocsSource = recoverySourceMetadata.getNumDocs();
+                final boolean softDeletesEnabled = shard.indexSettings().isSoftDeleteEnabled();
+                final long numDocsSource = softDeletesEnabled ? Lucene.getExactNumDocs(snapshot) : recoverySourceMetadata.getNumDocs();


same here just use getExactNumDocs

# Conflicts: # server/src/main/java/org/elasticsearch/indices/flush/SyncedFlushService.java

dnhatn · 2018-05-14T20:53:34Z

@elasticmachine test this please

dnhatn · 2018-05-15T17:13:04Z

We should just use exact numbers all the time and everywhere. keep it simple.

@bleskes I've updated this PR to always have an exact numDocs when capturing the store metadata snapshot. Would you please have a look? Thank you!

bleskes

LGTM. Thanks Nhat.

dnhatn · 2018-05-15T21:40:51Z

Thanks @bleskes and @s1monw

This test suite fails due to Lucene#getExactNumDocs. Relates #30228

Since #29458, we use a searcher to calculate the number of documents for a commit stats. Sadly, that approach is flawed. The searcher might no longer point to the last commit if it's refreshed. As synced-flush requires an exact numDocs to work correctly, we have to exclude all soft-deleted docs. This commit makes synced-flush stop using CommitStats but read an exact numDocs directly from an index commit. Relates #29458 Relates #29530

This PR adapts/utilizes recent enhancements in Lucene-7.4: - Replaces exactNumDocs by the soft-deletes count in SegmentCommitInfo. This enhancement allows us to back out changes introduced in #30228. - Always configure the soft-deletes field in IWC

dnhatn added >bug :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. labels Apr 28, 2018

dnhatn requested a review from s1monw April 28, 2018 03:00

dnhatn added 2 commits April 28, 2018 19:38

Optimize for no soft-deletes case

fe54296

Remove optimization

f479d3c

It might be incorrect if forceMerge happens

s1monw requested changes Apr 30, 2018

View reviewed changes

store soft-delete option in commit’s userdata

3480024

dnhatn requested review from bleskes and s1monw May 1, 2018 02:26

dnhatn added 2 commits April 30, 2018 22:28

Read local var

27119cd

test once is enough

67cdffa

dnhatn mentioned this pull request May 1, 2018

Add tombstone document into Lucene for Noop #30226

Merged

dnhatn added 2 commits April 30, 2018 23:25

Use the last commit if commit is null

9e4a98d

Use snapshotting commit for both

7827f41

dnhatn added 5 commits May 1, 2018 16:23

Calculate numDocs in engine only

577291d

Merge branch 'ccr' into fix-commit-stats

48eebf9

Make exact numDocs optional

e35c71c

Fix assertSameSyncIdSameDocs

15d0695

Merge branch 'ccr' into fix-commit-stats

911ca6e

dnhatn added a commit that referenced this pull request May 2, 2018

TEST: AwaitsFix assertSameSyncIdSameDocs

2da0cd2

This is tracked by #30228

dnhatn added 2 commits May 8, 2018 20:28

Merge branch 'ccr' into fix-commit-stats

dde0e3d

# Conflicts: # server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

Back out previous changes

bbfc223

dnhatn added a commit that referenced this pull request May 10, 2018

TEST: AwaitsFix assertSameSyncIdSameDocs

5beda41

This is tracked by #30228

dnhatn added 2 commits May 9, 2018 21:17

Merge branch 'ccr' into fix-commit-stats

80a8d73

Use exact numDocs in synced-flush

5ac26d4

bleskes reviewed May 10, 2018

View reviewed changes

s1monw approved these changes May 14, 2018

View reviewed changes

dnhatn added 2 commits May 14, 2018 11:02

do not need to check soft-deletes on source

34b19d0

Merge branch 'ccr' into fix-commit-stats

fa0c3bc

# Conflicts: # server/src/main/java/org/elasticsearch/indices/flush/SyncedFlushService.java

always get exact numdocs for snapshot

c42ca4b

bleskes approved these changes May 15, 2018

View reviewed changes

dnhatn changed the title ~~Exclude soft-deleted documents in commit stats~~ Use exact numDocs in synced-flush and store metadata snapshot May 15, 2018

dnhatn changed the title ~~Use exact numDocs in synced-flush and store metadata snapshot~~ Use exact numDocs in synced-flush and metadata snapshot May 15, 2018

dnhatn mentioned this pull request May 15, 2018

Use soft-deletes to maintain document history #29530

Closed

14 tasks

dnhatn merged commit b12c2f6 into elastic:ccr May 15, 2018

dnhatn deleted the fix-commit-stats branch May 15, 2018 22:00

dnhatn added the backport pending label May 15, 2018

dnhatn added a commit that referenced this pull request May 23, 2018

Mute CorruptedFileIT in CCR

65a5281

This test suite fails due to Lucene#getExactNumDocs. Relates #30228

dnhatn removed the backport pending label May 30, 2018

dnhatn mentioned this pull request Jun 5, 2018

Replace exact numDocs by soft-del count in SegmentCommitInfo #31086

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use exact numDocs in synced-flush and metadata snapshot #30228

Use exact numDocs in synced-flush and metadata snapshot #30228

dnhatn commented Apr 28, 2018 •

edited

Loading

elasticmachine commented Apr 28, 2018

dnhatn commented Apr 29, 2018

bleskes commented Apr 30, 2018

s1monw left a comment

s1monw Apr 30, 2018

dnhatn commented Apr 30, 2018

bleskes commented Apr 30, 2018

dnhatn commented Apr 30, 2018

dnhatn commented May 1, 2018

dnhatn commented May 1, 2018

s1monw commented May 3, 2018

s1monw commented May 3, 2018

dnhatn commented May 10, 2018

bleskes May 10, 2018

bleskes commented May 10, 2018

s1monw left a comment

s1monw May 14, 2018

s1monw May 14, 2018

s1monw May 14, 2018

dnhatn commented May 14, 2018

dnhatn commented May 15, 2018 •

edited

Loading

bleskes left a comment

dnhatn commented May 15, 2018

Use exact numDocs in synced-flush and metadata snapshot #30228

Use exact numDocs in synced-flush and metadata snapshot #30228

Conversation

dnhatn commented Apr 28, 2018 • edited Loading

elasticmachine commented Apr 28, 2018

dnhatn commented Apr 29, 2018

bleskes commented Apr 30, 2018

s1monw left a comment

Choose a reason for hiding this comment

s1monw Apr 30, 2018

Choose a reason for hiding this comment

dnhatn commented Apr 30, 2018

bleskes commented Apr 30, 2018

dnhatn commented Apr 30, 2018

dnhatn commented May 1, 2018

dnhatn commented May 1, 2018

s1monw commented May 3, 2018

s1monw commented May 3, 2018

dnhatn commented May 10, 2018

bleskes May 10, 2018

Choose a reason for hiding this comment

bleskes commented May 10, 2018

s1monw left a comment

Choose a reason for hiding this comment

s1monw May 14, 2018

Choose a reason for hiding this comment

s1monw May 14, 2018

Choose a reason for hiding this comment

s1monw May 14, 2018

Choose a reason for hiding this comment

dnhatn commented May 14, 2018

dnhatn commented May 15, 2018 • edited Loading

bleskes left a comment

Choose a reason for hiding this comment

dnhatn commented May 15, 2018

dnhatn commented Apr 28, 2018 •

edited

Loading

dnhatn commented May 15, 2018 •

edited

Loading